Re: [PATCH] rust: auxiliary: validate DeviceId name length

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Sep 09, 2026 at 01:29:18PM +0200, Danilo Krummrich wrote:
> On Wed Sep 9, 2026 at 9:19 AM CEST, Greg Kroah-Hartman wrote:
> > On Tue, Sep 08, 2026 at 11:32:46PM -0400, Georgios Androutsopoulos wrote:
> >> `DeviceId::new()` copies `modname` and `name` into the fixed 40-byte
> >> `auxiliary_device_id::name` array without checking that they fit. An
> >> oversized name is caught by the array bounds check, but the error
> >> reports an out-of-bounds index in the copy loop rather than the
> >> constraint the caller violated.
> >> 
> >> Check the invariant explicitly instead, so the failure states the length
> >> limit rather than an array index.
> >> 
> >> In a constant context exceeding the limit leads to a build error; at
> >> runtime it panics, so add a `# Panics` section for it.
> >> 
> >> Fixes: ce735e73dd59 ("rust: auxiliary: add auxiliary device / driver abstractions")
> 
> It's not actually fixing a bug, so I don't think this needs a Fixes: tag.
> 
> >> Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
> >> ---
> >>  rust/kernel/auxiliary.rs | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >> 
> >> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> >> index 60dfbec8f330..1f3ba86d6d96 100644
> >> --- a/rust/kernel/auxiliary.rs
> >> +++ b/rust/kernel/auxiliary.rs
> >> @@ -137,10 +137,20 @@ macro_rules! module_auxiliary_driver {
> >>  
> >>  impl DeviceId {
> >>      /// Create a new [`DeviceId`] from name.
> >> +    ///
> >> +    /// # Panics
> >> +    ///
> >> +    /// Panics if the combined module and device name, including the
> >> +    /// separator and trailing NUL, exceeds `AUXILIARY_NAME_SIZE` bytes.
> 
> I'd rather document that this is only intended to be called within device ID
> table creation; in const context a panic is just a compile time error.
> 
> >>      pub const fn new(modname: &'static CStr, name: &'static CStr) -> Self {
> >>          let name = name.to_bytes_with_nul();
> >>          let modname = modname.to_bytes_with_nul();
> >>  
> >> +        assert!(
> >> +            modname.len().saturating_add(name.len()) <= bindings::AUXILIARY_NAME_SIZE as usize,
> >> +            "auxiliary device ID is too long"
> >> +        );
> 
> Isn't this missing to consider the separator and NULL terminator?
> 
> >
> > We really shouldn't panic, we should error out and fail the creation
> > instead.
> 
> This is only ever used from const context to construct the device ID table, e.g.
> as in
> 
> 	kernel::auxiliary_device_table!(
> 	    AUX_TABLE,
> 	    <NovaDriver as auxiliary::Driver>::IdInfo,
> 	    [(
> 	        auxiliary::DeviceId::new(NOVA_CORE_MODULE_NAME, AUXILIARY_NAME),
> 	        ()
> 	    )]
> 	);
> 
> and a panic in const context makes the compilation fail, so it works as
> intended.
> 
> Unfortunately, we can't enforce that is function can only be called from const
> context, so technically it could also be called outside of the device ID table
> in non-const context, but it would be odd to construct outside of a device ID
> table.
> 
> > But what is placing the constraint of the name size here?  The C api
> > just takes a pointer, it doesn't care about the size, why does the rust
> > binding care?
> 
> I assume you were thinking of something else? This struct represents
> 
> 	#define AUXILIARY_NAME_SIZE 40
> 	
> 	struct auxiliary_device_id {
> 		char name[AUXILIARY_NAME_SIZE];
> 		kernel_ulong_t driver_data;
> 	};
> 

Ah, sorry, I was looking at auxiliary_device_create() which just takes a
pointer to a name, which is not the device_id, but the name by which the
device_id gets created from, my bad.

greg k-h