| Message ID | 20201012042404.2508-1-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid | expand |
Anant Thazhemadam wrote on Mon, Oct 12, 2020: > In p9_fd_create_unix, checking is performed to see if the addr (passed > as an argument) is NULL or not. > However, no check is performed to see if addr is a valid address, i.e., > it doesn't entirely consist of only 0's. > The initialization of sun_server.sun_path to be equal to this faulty > addr value leads to an uninitialized variable, as detected by KMSAN. > Checking for this (faulty addr) and returning a negative error number > appropriately, resolves this issue. I'm not sure I agree a fully zeroed address is faulty but I agree we can probably refuse it given userspace can't pass useful abstract addresses here. Just one nitpick but this is otherwise fine - good catch! > Reported-by: syzbot+75d51fe5bf4ebe988518@syzkaller.appspotmail.com > Tested-by: syzbot+75d51fe5bf4ebe988518@syzkaller.appspotmail.com > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> > --- > net/9p/trans_fd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index c0762a302162..8f528e783a6c 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -1023,7 +1023,7 @@ p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) > > csocket = NULL; > > - if (addr == NULL) > + if (!addr || !strlen(addr)) Since we don't care about the actual length here, how about checking for addr[0] directly? That'll spare a strlen() call in the valid case. Well, I guess it doesn't really matter -- I'll queue this up anyway and update if you resend. Thanks,
On 12-10-2020 13:29, Dominique Martinet wrote: > Anant Thazhemadam wrote on Mon, Oct 12, 2020: >> In p9_fd_create_unix, checking is performed to see if the addr (passed >> as an argument) is NULL or not. >> However, no check is performed to see if addr is a valid address, i.e., >> it doesn't entirely consist of only 0's. >> The initialization of sun_server.sun_path to be equal to this faulty >> addr value leads to an uninitialized variable, as detected by KMSAN. >> Checking for this (faulty addr) and returning a negative error number >> appropriately, resolves this issue. > I'm not sure I agree a fully zeroed address is faulty but I agree we can > probably refuse it given userspace can't pass useful abstract addresses > here. Understood. It's probably a better that I modify the commit message a little and send a v2 so it becomes more accurate. > Just one nitpick but this is otherwise fine - good catch! Thank you! > >> Reported-by: syzbot+75d51fe5bf4ebe988518@syzkaller.appspotmail.com >> Tested-by: syzbot+75d51fe5bf4ebe988518@syzkaller.appspotmail.com >> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> >> --- >> net/9p/trans_fd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c >> index c0762a302162..8f528e783a6c 100644 >> --- a/net/9p/trans_fd.c >> +++ b/net/9p/trans_fd.c >> @@ -1023,7 +1023,7 @@ p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) >> >> csocket = NULL; >> >> - if (addr == NULL) >> + if (!addr || !strlen(addr)) > Since we don't care about the actual length here, how about checking for > addr[0] directly? > That'll spare a strlen() call in the valid case. > You mentioned how a fully zeroed address isn't exactly faulty. By extension, wouldn't that mean that an address that simply begins with a 0 isn't faulty as well? This is an interesting point, because if the condition is modified to checking for addr[0] directly, addresses that simply begin with 0 (but have more non-zero content following) wouldn't be copied over either, right? In the end, it comes down to what you define as a "valid" value that sun_path can have. We've already agreed that a fully zeroed address wouldn't qualify as a valid value for sun_path. Are addresses that aren't fully zeroed, but only begin with a 0 also to be considered as an unacceptable value for sun_path? Thanks, Anant
Anant Thazhemadam wrote on Mon, Oct 12, 2020: > You mentioned how a fully zeroed address isn't exactly faulty. By extension, wouldn't that > mean that an address that simply begins with a 0 isn't faulty as well? That is correct. If you have a look at the unix(7) man page that describes AF_UNIX, it describes what 'abstract' addresses are and unix_mkname() in linux's net/unix/af_unix.c shows how it's handled. > This is an interesting point, because if the condition is modified to checking for addr[0] directly, > addresses that simply begin with 0 (but have more non-zero content following) wouldn't be > copied over either, right? Yes, we would reject any address that starts with a nul byte -- but that is already exactly what your patch does with strlen() already: a '\0' at the start of the string is equivalent to strlen(addr) == 0. The only difference is that checking for addr[0] won't run through all the string if it doesn't start with a nul byte; but this is a one-time thing at mount so it really doesn't matter. > In the end, it comes down to what you define as a "valid" value that sun_path can have. > We've already agreed that a fully zeroed address wouldn't qualify as a valid value for sun_path. > Are addresses that aren't fully zeroed, but only begin with a 0 also to be considered as an > unacceptable value for sun_path? Yes, because the strcpy() a few lines below would copy nothing, leaving sun_server.sun_path uninitialized like your example. At that point you could ask why not "fix" that strcpy to properly copy the address passed instead but that doesn't really make sense given where 'addr' comes from: it's passed from userspace as a nul-terminated string, so nothing after the first '\0' is valid. There probably are ways to work around that (e.g. iproute's ss will display abstract addresses with a leading '@' instead) but given nobody ever seemed to care I think it's safe to just return EINVAL there like you did ; there's nothing wrong with your patch as far as I'm concerned.
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index c0762a302162..8f528e783a6c 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -1023,7 +1023,7 @@ p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) csocket = NULL; - if (addr == NULL) + if (!addr || !strlen(addr)) return -EINVAL; if (strlen(addr) >= UNIX_PATH_MAX) {