> Date: Fri, 25 Aug 2023 13:38:02 -0400 > From: Theodore Preduta <t...@pta.gg> > > 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.
