general protection fault in sockfs_setattr

general protection fault in sockfs_setattr

[shankarapailoor]

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?

-- 
Regards,
Shankara Pailoor

Re: general protection fault in sockfs_setattr

[Cong Wang]

On Mon, Jun 4, 2018 at 9:53 PM, shankarapailoor
<shankarapailoor@gmail.com> 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;

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@gmail.com> wrote:
> On Mon, Jun 4, 2018 at 9:53 PM, shankarapailoor
> <shankarapailoor@gmail.com> 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

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@gmail.com> wrote:
>> On Mon, Jun 4, 2018 at 9:53 PM, shankarapailoor
>> <shankarapailoor@gmail.com> 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;
> 
> 
> 

Re: general protection fault in sockfs_setattr

[shankarapailoor]

++Al Viro

>Pastebin says that it was 4.17.0-rc4+ rather than 4.17-rc7.
Oops. Apologies Tetsuo. I confirmed the bug still happens with
linux.git. Here are the Syzkaller logs around the crash along with my
kernel configs.

Syzkaller Logs: https://pastebin.com/qqQyX0Ms
Config: https://pastebin.com/aEDARPDJ

Regards,
Shankara

On Wed, Jun 6, 2018 at 3:17 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> 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@gmail.com> wrote:
>>> On Mon, Jun 4, 2018 at 9:53 PM, shankarapailoor
>>> <shankarapailoor@gmail.com> 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

Re: general protection fault in sockfs_setattr

[Cong Wang]

On Tue, Jun 5, 2018 at 7:19 PM, shankarapailoor
<shankarapailoor@gmail.com> 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.

I know inode lock is held for ->setattr(), but not for ->release(), this
is why I suspect sock_close() could still race with sockfs_setattr()
after my patch.

I am not sure if it is crazy to just hold fd refcnt for fchmodat() too..