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+75d51fe5bf4ebe988...@syzkaller.appspotmail.com >> Tested-by: syzbot+75d51fe5bf4ebe988...@syzkaller.appspotmail.com >> Signed-off-by: Anant Thazhemadam <anant.thazhema...@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
