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.

- Added `iter_utf8` and `find_utf8` to `bytes::Cmdline`, 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.

- Fixed a parser bug that gemini pointed out with unmatched quotes,
  plus tests to check for that.

Signed-off-by: John Eckersberg <[email protected]>

Author: jeckersb

crates/kernel_cmdline/src/bytes.rs

@@ -5,6 +5,8 @@
 
 use std::borrow::Cow;
 
+use crate::utf8;
+
 use anyhow::Result;
 
 /// A parsed kernel command line.
@@ -64,6 +66,13 @@ impl<'a> Cmdline<'a> {
         CmdlineIter(&self.0)
     }
 
+    /// Returns an iterator over all parameters in the command line
+    /// which are valid UTF-8.
+    pub fn iter_utf8(&'a self) -> impl Iterator<Item = utf8::Parameter<'a>> {
+        self.iter()
+            .filter_map(|p| utf8::Parameter::try_from(p).ok())
+    }
+
     /// Locate a kernel argument with the given key name.
     ///
     /// Returns the first parameter matching the given key, or `None` if not found.
@@ -73,14 +82,32 @@ impl<'a> Cmdline<'a> {
         self.iter().find(|p| p.key == key)
     }
 
+    /// Locate a kernel argument with the given key name.
+    ///
+    /// Returns an error if a parameter with the given key name is
+    /// found, but the value is not valid UTF-8.
+    ///
+    /// Otherwise, returns the first parameter matching the given key,
+    /// or `None` if not found.  Key comparison treats dashes and
+    /// underscores as equivalent.
+    pub fn find_utf8(&'a self, key: impl AsRef<str>) -> Result<Option<utf8::Parameter<'a>>> {
+        let bytes = match self.find(key.as_ref()) {
+            Some(p) => p,
+            None => return Ok(None),
+        };
+
+        Ok(Some(utf8::Parameter::try_from(bytes)?))
+    }
+
     /// Find all kernel arguments starting with the given prefix.
     ///
     /// This is a variant of [`Self::find`].
-    pub fn find_all_starting_with(
+    pub fn find_all_starting_with<T: AsRef<[u8]> + ?Sized>(
         &'a self,
-        prefix: &'a [u8],
+        prefix: &'a T,
     ) -> impl Iterator<Item = Parameter<'a>> + 'a {
-        self.iter().filter(move |p| p.key.0.starts_with(prefix))
+        self.iter()
+            .filter(move |p| p.key.0.starts_with(prefix.as_ref()))
     }
 
     /// Locate the value of the kernel argument with the given key name.
@@ -199,11 +226,10 @@ impl<'a> Parameter<'a> {
                 value = &value[1..];
 
                 // *Only* the first and last double quotes are stripped
-                value = value
-                    .strip_prefix(b"\"")
-                    .unwrap_or(value)
-                    .strip_suffix(b"\"")
-                    .unwrap_or(value);
+                value = {
+                    let v = value.strip_prefix(b"\"").unwrap_or(value);
+                    v.strip_suffix(b"\"").unwrap_or(v)
+                };
 
                 Self {
                     key,
@@ -237,11 +263,15 @@ impl<'a> PartialEq for Parameter<'a> {
 mod tests {
     use super::*;
 
-    // convenience method for tests
+    // convenience methods for tests
     fn param(s: &str) -> Parameter<'_> {
         Parameter::parse(s.as_bytes()).0.unwrap()
     }
 
+    fn param_utf8(s: &str) -> utf8::Parameter<'_> {
+        utf8::Parameter::parse(s).0.unwrap()
+    }
+
     #[test]
     fn test_parameter_parse() {
         let (p, rest) = Parameter::parse(b"foo");
@@ -281,6 +311,12 @@ mod tests {
     fn test_parameter_quoted() {
         let p = param("foo=\"quoted value\"");
         assert_eq!(p.value, Some(b"quoted value".as_slice()));
+
+        let p = param("foo=\"unclosed quotes");
+        assert_eq!(p.value, Some(b"unclosed quotes".as_slice()));
+
+        let p = param("foo=trailing_quotes\"");
+        assert_eq!(p.value, Some(b"trailing_quotes".as_slice()));
     }
 
     #[test]
@@ -369,6 +405,38 @@ mod tests {
         assert!(kargs.find("nothing").is_none());
     }
 
+    #[test]
+    fn test_kargs_iter_utf8() {
+        let kargs = Cmdline::from(b"foo=bar,bar2 \xff baz=fuz bad=oh\xffno wiz");
+        let mut iter = kargs.iter_utf8();
+
+        assert_eq!(iter.next(), Some(param_utf8("foo=bar,bar2")));
+        assert_eq!(iter.next(), Some(param_utf8("baz=fuz")));
+        assert_eq!(iter.next(), Some(param_utf8("wiz")));
+        assert_eq!(iter.next(), None);
+    }
+
+    #[test]
+    fn test_kargs_find_utf8() {
+        let kargs = Cmdline::from(b"foo=bar,bar2 \xff baz=fuz bad=oh\xffno wiz");
+
+        // found it
+        assert_eq!(
+            kargs.find_utf8("foo").unwrap().unwrap().value().unwrap(),
+            "bar,bar2"
+        );
+
+        // didn't find it
+        assert!(kargs.find_utf8("nothing").unwrap().is_none());
+
+        // found it but key is invalid
+        let p = kargs.find_utf8("bad");
+        assert_eq!(
+            p.unwrap_err().to_string(),
+            "Parameter value is not valid UTF-8"
+        );
+    }
+
     #[test]
     fn test_kargs_from_proc() {
         let kargs = Cmdline::from_proc().unwrap();

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)
     }
 }
 
@@ -71,7 +73,8 @@ impl<'a> Cmdline<'a> {
     /// Returns the first parameter matching the given key, or `None` if not found.
     /// Key comparison treats dashes and underscores as equivalent.
     pub fn find(&'a self, key: impl AsRef<str>) -> Option<Parameter<'a>> {
-        self.iter().find(|p| p.key() == key.as_ref().into())
+        let key = ParameterKey::from(key.as_ref());
+        self.iter().find(|p| p.key() == key)
     }
 
     /// Find all kernel arguments starting with the given UTF-8 prefix.
@@ -122,15 +125,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 +188,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 +211,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 +358,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: &str = "rd.";
+/// The kernel argument for configuring the rootfs flags.
+pub const ROOTFLAGS: &str = "rootflags";
 
 const DEFAULT_REPO_CONFIG: &[(&str, &str)] = &[
     // Default to avoiding grub2-mkconfig etc.
@@ -1653,18 +1657,24 @@ 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_utf8("root")?
+        .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 +1842,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 +2095,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