Re: [PATCH 0/1] rust: configfs: Fix reference creation from uninitialized data in `Attribute::show`

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

On Fri Oct 9, 2026 at 9:32 PM BST, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
>
>> On Fri Oct 9, 2026 at 1:49 PM BST, Andreas Hindborg wrote:
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>
>>> Actually there may be a problem, but I think it may be in C configfs. If
>>> you take a look at the function that reads data from the iov_iter:
>>>
>>>   static int fill_write_buffer(struct configfs_buffer *buffer,
>>>             struct iov_iter *from)
>>>   {
>>>     int copied;
>>>
>>>     if (!buffer->page)
>>>       buffer->page = kmalloc(PAGE_SIZE, GFP_KERNEL); // <- HERE
>>>     if (!buffer->page)
>>>       return -ENOMEM;
>>>
>>>     copied = copy_from_iter(buffer->page, SIMPLE_ATTR_SIZE - 1, from);
>>>     buffer->needs_read_fill = 1;
>>>     /* if buf is assumed to contain a string, terminate it by \0,
>>>     * so e.g. sscanf() can scan the string easily */
>>>     buffer->page[copied] = 0;
>>>     return copied ? : -EFAULT;
>>>   }
>>>
>>> This function does not zero the page that is written into. This,
>>> combined with the buffer being per file handle means that you can read
>>> the original data in the page. The `kmalloc` should be probably be
>>> replaced with a `kzalloc`. This should be a problem for C modules as
>>> well.
>>
>> Why is this an issue? The extra uninitialized bytes should not be used.
>
> I guess you are right. In C, a driver just promise to not read beyond
> the write count. At any rate, it is a cheap defensive mechanism to
> kzalloc this buffer. It would prevent leaking uninitialized data to user
> space under a wrong length returned by the `show` implementation after a
> `store` operation.
>
> In rust it is an issue because the *driver* _may_ read the uninitialized
> bytes.

Well, the driver shouldn't read the bytes at all. It is supposed to *write* to
the slice. We probably want something like
https://docs.rs/tokio/latest/tokio/io/struct.ReadBuf.html 
instead of a raw slice.

(Also, the name "page" for variable name is a bad one, it leaks the
implementation detail of configfs. This is just a buffer to avoid having drivers
dealing with iov itself.)

>
> So we can fix it on the Rust side with this patch from Priya. Or we can
> initialize the buffer on allocation in C.

Not really, we can just "declare" all bytes allocated by kmalloc is initialized,
but to unpredictable values.

>
> For the record, I did not notice that the buffer is per file open, which
> is why the bug is here in the first place.
>
> @Breno, should we zero this buffer on initialization on the C side, or
> do we just zero the buffer before calling into Rust driver code?

Of course, leaking any bytes to userspace is bad because this is kernel memory.
So one can argue that zeroing it would be a good hardening measure.

Best,
Gary