[PATCH] rust: auxiliary: validate DeviceId name length

[PATCH] rust: auxiliary: validate DeviceId name length

[Georgios Androutsopoulos]

`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")
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.
     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"
+        );
+
         let mut id: bindings::auxiliary_device_id = pin_init::zeroed();
         let mut i = 0;
         while i < modname.len() {

base-commit: 28924df2a08f440c73991b83028032c901de2ae4
-- 
2.47.3

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

[Greg Kroah-Hartman]

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")
> 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.
>      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"
> +        );

We really shouldn't panic, we should error out and fail the creation
instead.

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?

thanks,

greg k-h

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

[Alexandre Courbot]

On Wed Sep 9, 2026 at 4:19 PM JST, 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")
>> 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.
>>      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"
>> +        );
>
> We really shouldn't panic, we should error out and fail the creation
> instead.
>
> 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?

Note that this is not a runtime panic, this method is only ever called
in const context by the `auxiliary_device_table!` macro, so a panic here
translates to a build error. The current code also panicks if the name
is larger than the target array, but it did so when the array was
accessed out-of-bounds, with a more obscure error message. What this
patch does is provide a better error message for a condition that was
already checked.

So while the patch is arguably an improvement, I would suggest to drop
its `Fixes:` tag as it doesn't really fixes a condition that wasn't
already checked. Also the commit message's "at runtime it panics" gives
the wrong idea of when this code is evaluated.

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

[Miguel Ojeda]

On Wed, Sep 9, 2026 at 1:29 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Note that this is not a runtime panic, this method is only ever called
> in const context by the `auxiliary_device_table!` macro, so a panic here
> translates to a build error.

Yeah, this is one instance where `#[const_eval_only]` (or
`#[consteval]` or ...) will make it clearer.

Cheers,
Miguel

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

[Gary Guo]

On Wed Sep 9, 2026 at 12:29 PM BST, Alexandre Courbot wrote:
> On Wed Sep 9, 2026 at 4:19 PM JST, 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")
>>> 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.
>>>      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"
>>> +        );
>>
>> We really shouldn't panic, we should error out and fail the creation
>> instead.
>>
>> 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?
>
> Note that this is not a runtime panic, this method is only ever called
> in const context by the `auxiliary_device_table!` macro, so a panic here
> translates to a build error. The current code also panicks if the name
> is larger than the target array, but it did so when the array was
> accessed out-of-bounds, with a more obscure error message. What this
> patch does is provide a better error message for a condition that was
> already checked.

Yeah, I think the message is good, however we don't need the panic annotation
for const-eval-only code.

Best,
Gary

>
> So while the patch is arguably an improvement, I would suggest to drop
> its `Fixes:` tag as it doesn't really fixes a condition that wasn't
> already checked. Also the commit message's "at runtime it panics" gives
> the wrong idea of when this code is evaluated.

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

[Danilo Krummrich]

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

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

[Greg Kroah-Hartman]

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

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

[George Androutsopoulos]

On Wed, Sep 9, 2026 at 7:29 AM Danilo Krummrich <dakr@kernel.org> wrote:
> It's not actually fixing a bug, so I don't think this needs a Fixes: tag.

Agreed, dropping it in v2.

> 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.

Agreed, replacing the `# Panics` section with that in v2.

> >>      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?

I believe both are covered. `to_bytes_with_nul()` includes each string's
terminator, and after the copy loop `modname`'s NUL is overwritten
with the `.` separator, so `modname.len() + name.len()` is exactly the
required capacity.

At the boundary, `DeviceId::new(c"a", <37 bytes>)` stores "a." plus 37
bytes plus the NUL, which is 1 + 1 + 37 + 1 = 40 and is accepted; one
byte more is rejected.

> 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.

Ideally that would be enforced rather than documented. Miguel pointed at
Gary's `#[const_eval_only]` [1] in the other branch, and `DeviceId::new()`
looks like a good candidate for it: it exists only to build device ID
tables, so a non-const call is a mistake rather than a supported use.

For v2 I will document the intent, and the attribute can be applied as a
follow-up once that series lands.

[1] https://lore.kernel.org/rust-for-linux/20260903-cv-v2-0-e93b1613e40c@garyguo.net/


On Wed, Sep 9, 2026 at 7:29 AM Danilo Krummrich <dakr@kernel.org> 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;
>         };