Messages in this thread | | | Subject | Re: [PATCH net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid | From | Anant Thazhemadam <> | Date | Mon, 12 Oct 2020 14:47:50 +0530 |
| |
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
| |