Re: CVS commit: src/sys/compat/linux/common

[Theodore Preduta <theo%pta.gg@localhost>]

On 2023-08-25 13:13, Taylor R Campbell wrote:
>> Module Name:    src
>> Committed By:   christos
>> Date:           Wed Aug 23 19:17:59 UTC 2023
>>
>> Modified Files:
>>         src/sys/compat/linux/common: linux_inotify.c
>>
>> Log Message:
>> put variable length structure at the end, so that clang does not complain.
>>
>>  struct inotify_entry {
>>         TAILQ_ENTRY(inotify_entry)      ie_entries;
>> +       char                            ie_name[NAME_MAX + 1];
>>         struct linux_inotify_event      ie_event;
>> -       char                            ie_name[NAME_MAX+1];
>>  };
> 
> This can't be right, and it's a little unsettling that the problem
> isn't caught by any automatic tests.
> 
> As I understand it, the `ie_name' member is supposed to provide
> storage for the `ie_event.name' array.
> 
> So this looks like this change is going to lead either to a buffer
> overrun -- reading someone else's heap memory, or scribbling all over
> someone else's heap memory -- or to uninitialized or zero-filled
> memory being published to userland where there should be a pathname.

I don't think so.  Notice that in inotify_read() two calls to uiomove()
are made, one on ie_event and one on ie_name... and in general this code
never accesses ie_event.name directly.

The reason I separated the fields in the first place was to make
allocation/deallocation simpler, not to use ie_name as storage for
ie_event.name.

Theo(dore)