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

["Gary Guo" <gary@garyguo.net>]

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.