lkml.org 
[lkml]   [2018]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: general protection fault in sockfs_setattr
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

\
 
 \ /
  Last update: 2018-06-06 04:19    [W:0.065 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site