Messages in this thread | | | Subject | Re: general protection fault in sockfs_setattr | From | Tetsuo Handa <> | Date | Wed, 6 Jun 2018 19:17:49 +0900 |
| |
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; > > >
| |