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

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Date: Fri, 25 Aug 2023 13:38:02 -0400
> From: Theodore Preduta <theo%pta.gg@localhost>
> 
> On 2023-08-25 13:13, Taylor R Campbell wrote:
> > 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.

Got it!  So that would explain why automatic tests didn't catch
anything.  Thanks!  Maybe we should have a comment about this to
clarify for future readers.

Side note: I realize there's a KASSERT protecting one of the strcpy
calls, and the other one is limited to copying from values of struct
dirent::d_name created by VOP_READDIR which should be limited to
NAME_MAX (plus NUL terminator).  But I'd be a little more comfortable
if we didn't use strcpy at all -- strlcpy is probably the right choice
here.  Might prefer to set buf->ie_event.len to MIN(strlen(name) + 1,
sizeof(buf->ie_name)) too, so that non-DIAGNOSTIC builds will truncate
the data rather than overrun the buffer.