Re: general protection fault in sockfs_setattr

[shankarapailoor]

Hi Cong,

I added that check and it seems to stop the crash. Like you said, I
don't see where the reference count for the file is increased. The
inode lock also seems to be held during this call.

Regards,
Shankara



On Tue, Jun 5, 2018 at 12:14 PM, Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
> On Mon, Jun 4, 2018 at 9:53 PM, shankarapailoor
> <shankarapailoor@xxxxxxxxx> wrote:
>> Hi,
>>
>> I have been fuzzing Linux 4.17-rc7 with Syzkaller and found the
>> following crash: https://pastebin.com/ixX3RB9j
>>
>> Syzkaller isolated the cause of the bug to the following program:
>>
>> socketpair$unix(0x1, 0x1, 0x0,
>> &(0x7f0000000000)={<r0=>0xffffffffffffffff, <r1=>0xffffffffffffffff})
>> getresuid(&(0x7f0000000080)=<r2=>0x0, &(0x7f00000000c0),
>> &(0x7f0000000700))r3 = getegid()
>> fchownat(r0, &(0x7f0000000040)='\x00', r2, r3, 0x1000)
>> dup3(r1, r0, 0x80000)
>>
>>
>> The problematic area appears to be here:
>>
>> static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr)
>> {
>>     int err = simple_setattr(dentry, iattr);
>>
>>     if (!err && (iattr->ia_valid & ATTR_UID)) {
>>          struct socket *sock = SOCKET_I(d_inode(dentry));
>>
>>          sock->sk->sk_uid = iattr->ia_uid; //KASAN GPF
>>     }
>>     return err;
>> }
>>
>> If dup3 is called concurrently with fchownat then can sock->sk be NULL?
>
> Although dup3() implies a close(), fd is refcnt'ted, if dup3() runs
> concurrently with fchownat() it should not be closed until whoever
> the last closes it.
>
> Or maybe fchownat() doesn't even hold refcnt of fd, since it aims
> to change the file backed.
>
>
> Not sure if the following is sufficient, inode might need to be protected
> with some lock...
>
> diff --git a/net/socket.c b/net/socket.c
> index f10f1d947c78..6294b4b3132e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -537,7 +537,10 @@ static int sockfs_setattr(struct dentry *dentry,
> struct iattr *iattr)
>         if (!err && (iattr->ia_valid & ATTR_UID)) {
>                 struct socket *sock = SOCKET_I(d_inode(dentry));
>
> -               sock->sk->sk_uid = iattr->ia_uid;
> +               if (sock->sk)
> +                       sock->sk->sk_uid = iattr->ia_uid;
> +               else
> +                       err = -ENOENT;
>         }
>
>         return err;



-- 
Regards,
Shankara Pailoor