Re: general protection fault in sockfs_setattr

[Tetsuo Handa]

Pastebin says that it was 4.17.0-rc4+ rather than 4.17-rc7.
I suggest reporting to Al Viro and linux-fsdevel ML after
confirming that this bug still happens with linux.git , in
case this is a dentry related bug (e.g. someone is by error
calling dput() without getting a refcount).

Also, please don't eliminate kernel messages prior to the
crash. Sometimes previous kernel messages (e.g. memory
allocation fault injection) as-is indicate the cause.

On 2018/06/06 11:19, shankarapailoor wrote:
> 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;
> 
> 
>