[net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid

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.

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(-)

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.