| Message ID | 20241115094908.3783952-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: add check for symlink corrupted | expand |
On Fri 15-11-24 17:49:08, Lizhi Xu wrote: > syzbot reported a null-ptr-deref in pick_link. [1] > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > it will trigger null pointer deref when accessing *res in pick_link(). > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > not symlink. > > [1] > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > CPU: 0 UID: 0 PID: 5310 Comm: syz-executor255 Not tainted 6.12.0-rc6-syzkaller-00318-ga9cda7c0ffed #0 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014 > RIP: 0010:pick_link+0x51c/0xd50 fs/namei.c:1864 Hum, based on line number is: if (*res == '/') { <<<< HERE error = nd_jump_root(nd); if (unlikely(error)) So res would be non-zero but a small number. > Code: c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 fc 00 e9 ff 48 8b 2b 48 85 ed 0f 84 92 00 00 00 e8 7b 36 7f ff 48 89 e8 48 c1 e8 03 <42> 0f b6 04 38 84 c0 0f 85 a2 05 00 00 0f b6 5d 00 bf 2f 00 00 00 > RSP: 0018:ffffc9000d147998 EFLAGS: 00010246 > RAX: 0000000000000000 RBX: ffff88804558dec8 RCX: ffff88801ec7a440 > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 > RBP: 0000000000000002 R08: ffffffff8215a35f R09: 1ffffffff203a13d > R10: dffffc0000000000 R11: fffffbfff203a13e R12: 1ffff92001a28f93 > R13: ffffc9000d147af8 R14: 1ffff92001a28f5f R15: dffffc0000000000 > FS: 0000555577611380(0000) GS:ffff88801fc00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007fcc0a595ed8 CR3: 0000000035760000 CR4: 0000000000352ef0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > step_into+0xca9/0x1080 fs/namei.c:1923 > lookup_last fs/namei.c:2556 [inline] > path_lookupat+0x16f/0x450 fs/namei.c:2580 > filename_lookup+0x256/0x610 fs/namei.c:2609 > user_path_at+0x3a/0x60 fs/namei.c:3016 > do_mount fs/namespace.c:3844 [inline] > __do_sys_mount fs/namespace.c:4057 [inline] > __se_sys_mount+0x297/0x3c0 fs/namespace.c:4034 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > RIP: 0033:0x7f4b18ad5b19 > Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 > RSP: 002b:00007ffc2e486c48 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5 > RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f4b18ad5b19 > RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000000 > RBP: 00007f4b18b685f0 R08: 0000000000000000 R09: 00005555776124c0 > R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffc2e486c70 > R13: 00007ffc2e486e98 R14: 431bde82d7b634db R15: 00007f4b18b1e03b > </TASK> > > Reported-and-tested-by: syzbot+73d8fc29ec7cba8286fa@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > --- > fs/namei.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/namei.c b/fs/namei.c > index 4a4a22a08ac2..f5dbccb3aafc 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1844,6 +1844,9 @@ static const char *pick_link(struct nameidata *nd, struct path *link, > if (unlikely(error)) > return ERR_PTR(error); > > + if (!S_ISLNK(inode->i_mode)) > + return ERR_PTR(-EINVAL); > + So I don't see how we can get here without inode being a symlink. pick_link() is called from step_into() which has among other things: if (likely(!d_is_symlink(path.dentry)) || ...) do something and return so we are checking whether the inode is a symlink before calling pick_link(). And yes, the d_is_symlink() is using cached type in dentry->d_flags so they could mismatch. But inode is not supposed to change its type during its lifetime so if there is a mismatch that is the problem that needs to be fixed. Honza
On Fri, Nov 15, 2024 at 05:49:08PM +0800, Lizhi Xu wrote: > syzbot reported a null-ptr-deref in pick_link. [1] > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > it will trigger null pointer deref when accessing *res in pick_link(). > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > not symlink. NAK. Don't paper over filesystem bugs at pathwalk time - it's the wrong place for that. Fix it at in-core inode creation time.
On Fri, Nov 15, 2024 at 01:06:15PM +0000, Al Viro wrote: > On Fri, Nov 15, 2024 at 05:49:08PM +0800, Lizhi Xu wrote: > > syzbot reported a null-ptr-deref in pick_link. [1] > > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > > it will trigger null pointer deref when accessing *res in pick_link(). > > > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > > not symlink. > > NAK. Don't paper over filesystem bugs at pathwalk time - it's the wrong > place for that. Fix it at in-core inode creation time. BTW, seeing that ntfs doesn't even touch ->i_link, you are dealing with aftermath of memory corruption, so it's definitely papering over the actual bug here.
On Fri, 15 Nov 2024 12:43:06 +0100, Jan Kara wrote: > On Fri 15-11-24 17:49:08, Lizhi Xu wrote: > > syzbot reported a null-ptr-deref in pick_link. [1] > > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > > it will trigger null pointer deref when accessing *res in pick_link(). > > > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > > not symlink. > > > > [1] > > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > > CPU: 0 UID: 0 PID: 5310 Comm: syz-executor255 Not tainted 6.12.0-rc6-syzkaller-00318-ga9cda7c0ffed #0 > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014 > > RIP: 0010:pick_link+0x51c/0xd50 fs/namei.c:1864 > > Hum, based on line number is: > > if (*res == '/') { <<<< HERE > error = nd_jump_root(nd); > if (unlikely(error)) > > So res would be non-zero but a small number. > > > Code: c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 fc 00 e9 ff 48 8b 2b 48 85 ed 0f 84 92 00 00 00 e8 7b 36 7f ff 48 89 e8 48 c1 e8 03 <42> 0f b6 04 38 84 c0 0f 85 a2 05 00 00 0f b6 5d 00 bf 2f 00 00 00 > > RSP: 0018:ffffc9000d147998 EFLAGS: 00010246 > > RAX: 0000000000000000 RBX: ffff88804558dec8 RCX: ffff88801ec7a440 > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 > > RBP: 0000000000000002 R08: ffffffff8215a35f R09: 1ffffffff203a13d > > R10: dffffc0000000000 R11: fffffbfff203a13e R12: 1ffff92001a28f93 > > R13: ffffc9000d147af8 R14: 1ffff92001a28f5f R15: dffffc0000000000 > > FS: 0000555577611380(0000) GS:ffff88801fc00000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00007fcc0a595ed8 CR3: 0000000035760000 CR4: 0000000000352ef0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > Call Trace: > > <TASK> > > step_into+0xca9/0x1080 fs/namei.c:1923 > > lookup_last fs/namei.c:2556 [inline] > > path_lookupat+0x16f/0x450 fs/namei.c:2580 > > filename_lookup+0x256/0x610 fs/namei.c:2609 > > user_path_at+0x3a/0x60 fs/namei.c:3016 > > do_mount fs/namespace.c:3844 [inline] > > __do_sys_mount fs/namespace.c:4057 [inline] > > __se_sys_mount+0x297/0x3c0 fs/namespace.c:4034 > > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > > do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 > > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > RIP: 0033:0x7f4b18ad5b19 > > Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 > > RSP: 002b:00007ffc2e486c48 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5 > > RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f4b18ad5b19 > > RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000000 > > RBP: 00007f4b18b685f0 R08: 0000000000000000 R09: 00005555776124c0 > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffc2e486c70 > > R13: 00007ffc2e486e98 R14: 431bde82d7b634db R15: 00007f4b18b1e03b > > </TASK> > > > > Reported-and-tested-by: syzbot+73d8fc29ec7cba8286fa@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> > > --- > > fs/namei.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 4a4a22a08ac2..f5dbccb3aafc 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -1844,6 +1844,9 @@ static const char *pick_link(struct nameidata *nd, struct path *link, > > if (unlikely(error)) > > return ERR_PTR(error); > > > > + if (!S_ISLNK(inode->i_mode)) > > + return ERR_PTR(-EINVAL); > > + > > So I don't see how we can get here without inode being a symlink. > pick_link() is called from step_into() which has among other things: > > if (likely(!d_is_symlink(path.dentry)) || ...) Our idea is the same. Because d_is_symlink() has confirmed the mode of symlink in step_into(), I will confirm whether the mode of symlink's inode has changed when the value of i_link is 2 in pick_link(). > do something and return > > so we are checking whether the inode is a symlink before calling > pick_link(). And yes, the d_is_symlink() is using cached type in > dentry->d_flags so they could mismatch. But inode is not supposed to change > its type during its lifetime so if there is a mismatch that is the problem > that needs to be fixed. I think syzbot executed the following two syscalls when triggering this problem: link(&(0x7f0000000200)='./file0\x00', &(0x7f0000000240)='./bus\x00') mount$overlay(0x0, &(0x7f00000000c0)='./bus\x00', 0x0, 0x0, 0x0) Obviously, this is to mount a link. Whether the mount operation itself will change or corrupt the i_link value and mode value of the symlink is not clear to me yet. BR, Lizhi
On Sat, Nov 16, 2024 at 09:02:07AM +0800, Lizhi Xu wrote: > Our idea is the same. Because d_is_symlink() has confirmed the mode of > symlink in step_into(), I will confirm whether the mode of symlink's inode > has changed when the value of i_link is 2 in pick_link(). > > do something and return > > > > so we are checking whether the inode is a symlink before calling > > pick_link(). And yes, the d_is_symlink() is using cached type in > > dentry->d_flags so they could mismatch. But inode is not supposed to change > > its type during its lifetime so if there is a mismatch that is the problem > > that needs to be fixed. > I think syzbot executed the following two syscalls when triggering this problem: > > link(&(0x7f0000000200)='./file0\x00', &(0x7f0000000240)='./bus\x00') > mount$overlay(0x0, &(0x7f00000000c0)='./bus\x00', 0x0, 0x0, 0x0) > > Obviously, this is to mount a link. Whether the mount operation itself will > change or corrupt the i_link value and mode value of the symlink is not > clear to me yet. Odds are, it's not a valid struct inode instance in the first place. It's not inode->i_link that is a problem (*nothing* should ever store that value in there and ntfs doesn't even try that - grep and you'll see); it's inode itself. Have you tried KASAN-enabled build? Might be interesting to see if it catches anything...
On Fri, 15 Nov 2024 13:24:55 +0000, Al Viro wrote: > On Fri, Nov 15, 2024 at 01:06:15PM +0000, Al Viro wrote: > > On Fri, Nov 15, 2024 at 05:49:08PM +0800, Lizhi Xu wrote: > > > syzbot reported a null-ptr-deref in pick_link. [1] > > > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > > > it will trigger null pointer deref when accessing *res in pick_link(). > > > > > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > > > not symlink. > > > > NAK. Don't paper over filesystem bugs at pathwalk time - it's the wrong > > place for that. Fix it at in-core inode creation time. > > BTW, seeing that ntfs doesn't even touch ->i_link, you are dealing Yes, ntfs3 does not handle the relevant code of i_link. > with aftermath of memory corruption, so it's definitely papering over > the actual bug here. I see that finding out how the value of i_link becomes 2 is the key.
On Sat, Nov 16, 2024 at 09:39:50AM +0800, Lizhi Xu wrote: > On Fri, 15 Nov 2024 13:24:55 +0000, Al Viro wrote: > > On Fri, Nov 15, 2024 at 01:06:15PM +0000, Al Viro wrote: > > > On Fri, Nov 15, 2024 at 05:49:08PM +0800, Lizhi Xu wrote: > > > > syzbot reported a null-ptr-deref in pick_link. [1] > > > > When symlink's inode is corrupted, the value of the i_link is 2 in this case, > > > > it will trigger null pointer deref when accessing *res in pick_link(). > > > > > > > > To avoid this issue, add a check for inode mode, return -EINVAL when it's > > > > not symlink. > > > > > > NAK. Don't paper over filesystem bugs at pathwalk time - it's the wrong > > > place for that. Fix it at in-core inode creation time. > > > > BTW, seeing that ntfs doesn't even touch ->i_link, you are dealing > Yes, ntfs3 does not handle the relevant code of i_link. > > with aftermath of memory corruption, so it's definitely papering over > > the actual bug here. > I see that finding out how the value of i_link becomes 2 is the key. How about 'how the memory currently pointed to by inode had come to be available for use by something that stored 2 at that particular offset'?
diff --git a/fs/namei.c b/fs/namei.c index 4a4a22a08ac2..f5dbccb3aafc 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1844,6 +1844,9 @@ static const char *pick_link(struct nameidata *nd, struct path *link, if (unlikely(error)) return ERR_PTR(error); + if (!S_ISLNK(inode->i_mode)) + return ERR_PTR(-EINVAL); + res = READ_ONCE(inode->i_link); if (!res) { const char * (*get)(struct dentry *, struct inode *,
syzbot reported a null-ptr-deref in pick_link. [1] When symlink's inode is corrupted, the value of the i_link is 2 in this case, it will trigger null pointer deref when accessing *res in pick_link(). To avoid this issue, add a check for inode mode, return -EINVAL when it's not symlink. [1] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] CPU: 0 UID: 0 PID: 5310 Comm: syz-executor255 Not tainted 6.12.0-rc6-syzkaller-00318-ga9cda7c0ffed #0 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014 RIP: 0010:pick_link+0x51c/0xd50 fs/namei.c:1864 Code: c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 fc 00 e9 ff 48 8b 2b 48 85 ed 0f 84 92 00 00 00 e8 7b 36 7f ff 48 89 e8 48 c1 e8 03 <42> 0f b6 04 38 84 c0 0f 85 a2 05 00 00 0f b6 5d 00 bf 2f 00 00 00 RSP: 0018:ffffc9000d147998 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff88804558dec8 RCX: ffff88801ec7a440 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: 0000000000000002 R08: ffffffff8215a35f R09: 1ffffffff203a13d R10: dffffc0000000000 R11: fffffbfff203a13e R12: 1ffff92001a28f93 R13: ffffc9000d147af8 R14: 1ffff92001a28f5f R15: dffffc0000000000 FS: 0000555577611380(0000) GS:ffff88801fc00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fcc0a595ed8 CR3: 0000000035760000 CR4: 0000000000352ef0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> step_into+0xca9/0x1080 fs/namei.c:1923 lookup_last fs/namei.c:2556 [inline] path_lookupat+0x16f/0x450 fs/namei.c:2580 filename_lookup+0x256/0x610 fs/namei.c:2609 user_path_at+0x3a/0x60 fs/namei.c:3016 do_mount fs/namespace.c:3844 [inline] __do_sys_mount fs/namespace.c:4057 [inline] __se_sys_mount+0x297/0x3c0 fs/namespace.c:4034 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7f4b18ad5b19 Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007ffc2e486c48 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5 RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f4b18ad5b19 RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000000 RBP: 00007f4b18b685f0 R08: 0000000000000000 R09: 00005555776124c0 R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffc2e486c70 R13: 00007ffc2e486e98 R14: 431bde82d7b634db R15: 00007f4b18b1e03b </TASK> Reported-and-tested-by: syzbot+73d8fc29ec7cba8286fa@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> --- fs/namei.c | 3 +++ 1 file changed, 3 insertions(+)