kernel_cmdline: More improvements and fixes

- Removed `From<bytes::Parameter>` implementation for
  `utf8::Parameter` and similar for `utf8::ParameterKey`.  This was
  public and would allow end-users to construct utf8 parameters from
  non-utf8 data.  Replaced internally with `from_bytes` in the places
  where we know we can safely convert known-UTF-8 data.

- Added `TryFrom<bytes::Paramter>` implementation for
  `utf8::Parameter` to allow checked conversions, plus tests.

- Updated `find_root_args_to_inherit` in bootc to use these
  improvements.  Notably bootc will now allow non-UTF8 data in the
  kernel cmdline, *unless* it occurs in parameters that bootc is
  explicitly looking for.

- Added more tests to `find_root_args_to_inherit` to validate expected
  functionality with non-UTF-8 data.

Signed-off-by: John Eckersberg <jeckersb@redhat.com>

Author: jeckersb

crates/kernel_cmdline/src/lib.rs

@@ -12,8 +12,3 @@
 
 pub mod bytes;
 pub mod utf8;
-
-/// This is used by dracut.
-pub const INITRD_ARG_PREFIX: &str = "rd.";
-/// The kernel argument for configuring the rootfs flags.
-pub const ROOTFLAGS: &str = "rootflags";

crates/kernel_cmdline/src/utf8.rs

@@ -3,6 +3,8 @@
 //! This module provides functionality for parsing and working with kernel command line
 //! arguments, supporting both key-only switches and key-value pairs with proper quote handling.
 
+use std::ops::Deref;
+
 use crate::bytes;
 
 use anyhow::Result;
@@ -34,7 +36,7 @@ impl<'a> Iterator for CmdlineIter<'a> {
     type Item = Parameter<'a>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        self.0.next().map(Into::into)
+        self.0.next().map(Parameter::from_bytes)
     }
 }
 
@@ -122,15 +124,19 @@ impl<'a> std::ops::Deref for ParameterKey<'a> {
     }
 }
 
-impl<'a> From<&'a str> for ParameterKey<'a> {
-    fn from(input: &'a str) -> Self {
-        Self(bytes::ParameterKey(input.as_bytes()))
+impl<'a> ParameterKey<'a> {
+    /// Construct a utf8::ParameterKey from a bytes::ParameterKey
+    ///
+    /// This is non-public and should only be used when the underlying
+    /// bytes are known to be valid UTF-8.
+    fn from_bytes(input: bytes::ParameterKey<'a>) -> Self {
+        Self(input)
     }
 }
 
-impl<'a> From<bytes::ParameterKey<'a>> for ParameterKey<'a> {
-    fn from(input: bytes::ParameterKey<'a>) -> Self {
-        Self(input)
+impl<'a> From<&'a str> for ParameterKey<'a> {
+    fn from(input: &'a str) -> Self {
+        Self(bytes::ParameterKey(input.as_bytes()))
     }
 }
 
@@ -181,9 +187,17 @@ impl<'a> Parameter<'a> {
         }
     }
 
+    /// Construct a utf8::Parameter from a bytes::Parameter
+    ///
+    /// This is non-public and should only be used when the underlying
+    /// bytes are known to be valid UTF-8.
+    fn from_bytes(bytes: bytes::Parameter<'a>) -> Self {
+        Self(bytes)
+    }
+
     /// Returns the key part of the parameter
     pub fn key(&'a self) -> ParameterKey<'a> {
-        self.0.key().into()
+        ParameterKey::from_bytes(self.0.key())
     }
 
     /// Returns the optional value part of the parameter
@@ -196,9 +210,21 @@ impl<'a> Parameter<'a> {
     }
 }
 
-impl<'a> From<bytes::Parameter<'a>> for Parameter<'a> {
-    fn from(bytes: bytes::Parameter<'a>) -> Self {
-        Self(bytes)
+impl<'a> TryFrom<bytes::Parameter<'a>> for Parameter<'a> {
+    type Error = anyhow::Error;
+
+    fn try_from(bytes: bytes::Parameter<'a>) -> Result<Self, Self::Error> {
+        if str::from_utf8(bytes.key().deref()).is_err() {
+            anyhow::bail!("Parameter key is not valid UTF-8");
+        }
+
+        if let Some(value) = bytes.value() {
+            if str::from_utf8(value).is_err() {
+                anyhow::bail!("Parameter value is not valid UTF-8");
+            }
+        }
+
+        Ok(Self(bytes))
     }
 }
 
@@ -331,6 +357,37 @@ mod tests {
         assert_ne!(switch, keyvalue);
     }
 
+    #[test]
+    fn test_parameter_tryfrom() {
+        // ok switch
+        let p = bytes::Parameter::parse(b"foo").0.unwrap();
+        let utf = Parameter::try_from(p).unwrap();
+        assert_eq!(utf.key(), "foo".into());
+        assert_eq!(utf.value(), None);
+
+        // ok key/value
+        let p = bytes::Parameter::parse(b"foo=bar").0.unwrap();
+        let utf = Parameter::try_from(p).unwrap();
+        assert_eq!(utf.key(), "foo".into());
+        assert_eq!(utf.value(), Some("bar".into()));
+
+        // bad switch
+        let p = bytes::Parameter::parse(b"f\xffoo").0.unwrap();
+        let e = Parameter::try_from(p);
+        assert_eq!(
+            e.unwrap_err().to_string(),
+            "Parameter key is not valid UTF-8"
+        );
+
+        // bad key/value
+        let p = bytes::Parameter::parse(b"foo=b\xffar").0.unwrap();
+        let e = Parameter::try_from(p);
+        assert_eq!(
+            e.unwrap_err().to_string(),
+            "Parameter value is not valid UTF-8"
+        );
+    }
+
     #[test]
     fn test_kargs_simple() {
         // example taken lovingly from:

crates/lib/src/install.rs

@@ -62,7 +62,7 @@ use crate::spec::ImageReference;
 use crate::store::Storage;
 use crate::task::Task;
 use crate::utils::sigpolicy_from_opt;
-use bootc_kernel_cmdline::utf8::Cmdline;
+use bootc_kernel_cmdline::{bytes, utf8};
 use bootc_mount::Filesystem;
 
 /// The toplevel boot directory
@@ -83,6 +83,10 @@ const SELINUXFS: &str = "/sys/fs/selinux";
 /// The mount path for uefi
 const EFIVARFS: &str = "/sys/firmware/efi/efivars";
 pub(crate) const ARCH_USES_EFI: bool = cfg!(any(target_arch = "x86_64", target_arch = "aarch64"));
+/// This is used by dracut.
+pub const INITRD_ARG_PREFIX: &[u8] = b"rd.";
+/// The kernel argument for configuring the rootfs flags.
+pub const ROOTFLAGS: &[u8] = b"rootflags";
 
 const DEFAULT_REPO_CONFIG: &[(&str, &str)] = &[
     // Default to avoiding grub2-mkconfig etc.
@@ -1653,18 +1657,26 @@ struct RootMountInfo {
 
 /// Discover how to mount the root filesystem, using existing kernel arguments and information
 /// about the root mount.
-fn find_root_args_to_inherit(cmdline: &Cmdline, root_info: &Filesystem) -> Result<RootMountInfo> {
+fn find_root_args_to_inherit(
+    cmdline: &bytes::Cmdline,
+    root_info: &Filesystem,
+) -> Result<RootMountInfo> {
     // If we have a root= karg, then use that
-    let (mount_spec, kargs) = if let Some(root) = cmdline.value_of("root") {
-        let rootflags = cmdline.find(bootc_kernel_cmdline::ROOTFLAGS);
-        let inherit_kargs = cmdline.find_all_starting_with(bootc_kernel_cmdline::INITRD_ARG_PREFIX);
+    let root = cmdline
+        .find("root")
+        .map(utf8::Parameter::try_from)
+        .transpose()?
+        .and_then(|p| p.value().map(|p| p.to_string()));
+    let (mount_spec, kargs) = if let Some(root) = root {
+        let rootflags = cmdline.find(ROOTFLAGS);
+        let inherit_kargs = cmdline.find_all_starting_with(INITRD_ARG_PREFIX);
         (
-            root.to_owned(),
+            root,
             rootflags
                 .into_iter()
                 .chain(inherit_kargs)
-                .map(|p| p.to_string())
-                .collect(),
+                .map(|p| utf8::Parameter::try_from(p).map(|p| p.to_string()))
+                .collect::<Result<Vec<_>, _>>()?,
         )
     } else {
         let uuid = root_info
@@ -1832,7 +1844,7 @@ pub(crate) async fn install_to_filesystem(
         }
     } else if targeting_host_root {
         // In the to-existing-root case, look at /proc/cmdline
-        let cmdline = Cmdline::from_proc()?;
+        let cmdline = bytes::Cmdline::from_proc()?;
         find_root_args_to_inherit(&cmdline, &inspect)?
     } else {
         // Otherwise, gather metadata from the provided root and use its provided UUID as a
@@ -2085,18 +2097,42 @@ mod tests {
             uuid: Some("965eb3c7-5a3f-470d-aaa2-1bcf04334bc6".into()),
             children: None,
         };
-        let kargs = Cmdline::from("");
+        let kargs = bytes::Cmdline::from("");
         let r = find_root_args_to_inherit(&kargs, &inspect).unwrap();
         assert_eq!(r.mount_spec, "UUID=965eb3c7-5a3f-470d-aaa2-1bcf04334bc6");
 
-        let kargs =
-            Cmdline::from("root=/dev/mapper/root rw someother=karg rd.lvm.lv=root systemd.debug=1");
+        let kargs = bytes::Cmdline::from(
+            "root=/dev/mapper/root rw someother=karg rd.lvm.lv=root systemd.debug=1",
+        );
 
         // In this case we take the root= from the kernel cmdline
         let r = find_root_args_to_inherit(&kargs, &inspect).unwrap();
         assert_eq!(r.mount_spec, "/dev/mapper/root");
         assert_eq!(r.kargs.len(), 1);
         assert_eq!(r.kargs[0], "rd.lvm.lv=root");
+
+        // non-UTF8 data in non-essential parts of the cmdline should be ignored
+        let kargs = bytes::Cmdline::from(
+            b"root=/dev/mapper/root rw non-utf8=\xff rd.lvm.lv=root systemd.debug=1",
+        );
+        let r = find_root_args_to_inherit(&kargs, &inspect).unwrap();
+        assert_eq!(r.mount_spec, "/dev/mapper/root");
+        assert_eq!(r.kargs.len(), 1);
+        assert_eq!(r.kargs[0], "rd.lvm.lv=root");
+
+        // non-UTF8 data in `root` should fail
+        let kargs = bytes::Cmdline::from(
+            b"root=/dev/mapper/ro\xffot rw non-utf8=\xff rd.lvm.lv=root systemd.debug=1",
+        );
+        let r = find_root_args_to_inherit(&kargs, &inspect);
+        assert!(r.is_err());
+
+        // non-UTF8 data in `rd.` should fail
+        let kargs = bytes::Cmdline::from(
+            b"root=/dev/mapper/root rw non-utf8=\xff rd.lvm.lv=ro\xffot systemd.debug=1",
+        );
+        let r = find_root_args_to_inherit(&kargs, &inspect);
+        assert!(r.is_err());
     }
 
     // As this is a unit test we don't try to test mountpoints, just verify