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

[George Androutsopoulos]

On Wed, Sep 9, 2026 at 7:29 AM Danilo Krummrich <dakr@xxxxxxxxxx> 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@xxxxxxxxxxx/


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