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

["Danilo Krummrich" <dakr@kernel.org>]

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;
	};