Re: general protection fault in security_inode_getattr

From: Andrey Kalachev
Date: Fri Apr 19 2024 - 16:10:54 EST

Next message: Kirill A. Shutemov: "[PATCHv2] x86/tdx: Preserve shared bit on mprotect()"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 05/13] x86/irq: Reserve a per CPU IDT vector for posted MSIs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Apr 02, 2024 at 07:01:57PM +0300, Amir Goldstein wrote:

On Mon, Apr 1, 2024 at 8:43 PM Andrey Kalachev <kalachev@xxxxxxxxx> wrote:

On Wed, Jul 29, 2020 at 01:23:18PM -0700, syzbot wrote:
>Hello,
>
>syzbot found the following issue on:
>
>HEAD commit: 92ed3019 Linux 5.8-rc7
>git tree: upstream
>console output: https://syzkaller.appspot.com/x/log.txt?x=140003ac900000
>kernel config: https://syzkaller.appspot.com/x/.config?x=84f076779e989e69
>dashboard link: https://syzkaller.appspot.com/bug?extid=f07cc9be8d1d226947ed
>compiler: gcc (GCC) 10.1.0-syz 20200507
>
>Unfortunately, I don't have any reproducer for this issue yet.
>
>IMPORTANT: if you fix the issue, please add the following tag to the commit:
>Reported-by: syzbot+f07cc9be8d1d226947ed@xxxxxxxxxxxxxxxxxxxxxxxxx
>
>general protection fault, probably for non-canonical address 0xdffffc000000000c: 0000 [#1] PREEMPT SMP KASAN
>KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
>CPU: 0 PID: 9214 Comm: syz-executor.3 Not tainted 5.8.0-rc7-syzkaller #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
>RIP: 0010:security_inode_getattr+0x46/0x140 security/security.c:1276
>Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 04 01 00 00 48 b8 00 00 00 00 00 fc ff df 49 8b 5d 08 48 8d 7b 60 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 d7 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b
>RSP: 0018:ffffc9000d41f638 EFLAGS: 00010206
>RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffc9000f539000
>RDX: 000000000000000c RSI: ffffffff8354f8ee RDI: 0000000000000060
>RBP: ffffc9000d41f810 R08: 0000000000000001 R09: ffff88804edc2dc8
>R10: 0000000000000000 R11: 00000000000ebc58 R12: ffff888089f10170
>R13: ffffc9000d41f810 R14: 00000000000007ff R15: 0000000000000000
>FS: 00007f3599717700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 0000001b2c12c000 CR3: 0000000099919000 CR4: 00000000001406f0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>Call Trace:
> vfs_getattr+0x22/0x60 fs/stat.c:121
> ovl_copy_up_one+0x13b/0x1870 fs/overlayfs/copy_up.c:850
> ovl_copy_up_flags+0x14b/0x1d0 fs/overlayfs/copy_up.c:931
> ovl_maybe_copy_up+0x140/0x190 fs/overlayfs/copy_up.c:963
> ovl_open+0xba/0x270 fs/overlayfs/file.c:147
> do_dentry_open+0x501/0x1290 fs/open.c:828
> do_open fs/namei.c:3243 [inline]
> path_openat+0x1bb9/0x2750 fs/namei.c:3360
> do_filp_open+0x17e/0x3c0 fs/namei.c:3387
> file_open_name+0x290/0x400 fs/open.c:1124
> acct_on+0x78/0x770 kernel/acct.c:207
> __do_sys_acct kernel/acct.c:286 [inline]
> __se_sys_acct kernel/acct.c:273 [inline]
> __x64_sys_acct+0xab/0x1f0 kernel/acct.c:273
> do_syscall_64+0x60/0xe0 arch/x86/entry/common.c:384
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>RIP: 0033:0x45c369
>Code: 8d b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 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 0f 83 5b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>RSP: 002b:00007f3599716c78 EFLAGS: 00000246 ORIG_RAX: 00000000000000a3
>RAX: ffffffffffffffda RBX: 0000000000000700 RCX: 000000000045c369
>RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000440
>RBP: 000000000078bf30 R08: 0000000000000000 R09: 0000000000000000
>R10: 0000000000000000 R11: 0000000000000246 R12: 000000000078bf0c
>R13: 00007ffda41ffbef R14: 00007f35997179c0 R15: 000000000078bf0c
>Modules linked in:
>---[ end trace d1398a63985d3915 ]---
>RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
>RIP: 0010:security_inode_getattr+0x46/0x140 security/security.c:1276
>Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 04 01 00 00 48 b8 00 00 00 00 00 fc ff df 49 8b 5d 08 48 8d 7b 60 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 d7 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b
>RSP: 0018:ffffc9000d41f638 EFLAGS: 00010206
>RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffc9000f539000
>RDX: 000000000000000c RSI: ffffffff8354f8ee RDI: 0000000000000060
>RBP: ffffc9000d41f810 R08: 0000000000000001 R09: ffff88804edc2dc8
>R10: 0000000000000000 R11: 00000000000ebc58 R12: ffff888089f10170
>R13: ffffc9000d41f810 R14: 00000000000007ff R15: 0000000000000000
>FS: 00007f3599717700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 0000000020000440 CR3: 0000000099919000 CR4: 00000000001406f0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
>
>---
>This report is generated by a bot. It may contain errors.
>See https://goo.gl/tpsmEJ for more information about syzbot.
>syzbot engineers can be reached at syzkaller@xxxxxxxxxxxxxxxx.
>
>syzbot will keep track of this issue. See:
>https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

Hello,

I've found that the bug fixed by commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0af950f57fefabab628f1963af881e6b9bfe7f38
merged with mainline here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?id=be3c213150dc4370ef211a78d78457ff166eba4e

Kernel release 6.5 include the fixed code.

Hence, the stable kernels up to 6.5 still affected.
I've got early version (4.19.139) from syzbot report, here is the first time when been reported.
Maybe previous versions are also affected, I haven't checked it.

I've only deal with stable 5.10 and 6.1, here I can confirm the issue.

The tracing results showed that GPF caused by the dentry shared between two processes.
Suppose we have a regular file `A` onto lower overlayfs layer, metacopy=on.
P1 execute link syscall ( `A` link to `B`), P2 do open `B`.

P1 P2

sys_link
sys_open
ovl_lookup B -- lookup non existent `B`, alloc `B` dentry
ovl_alloc_entry -- non existent file, zero filled ovl_entry

ovl_link -- link A to B, use same dentry `B`, dentry associated with
`A`, lower layer file now.

sys_link -- return to userspace, zero filled ovl_entry `B` untouched

ovl_open B, reuse the same dentry `B`
ovl_copy_up_one
ovl_path_lower
ovl_numlower(oe) -- return 0, numlower in zero filled ovl_entry `oe`
ovl_path_lower -- return zero filled `struct path`
vfs_getattr(struct path, ..)
security_inode_getattr(struct path, ...)
d_backing_inode(path->dentry) -- NULL dereference, GPF

Stable kernel v6.1 can be easy fixed by 4 mainline commits transfer:

0af950f57fef ovl: move ovl_entry into ovl_inode
163db0da3515 ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
5522c9c7cbd2 ovl: use ovl_numlower() and ovl_lowerstack() accessors
a6ff2bc0be17 ovl: use OVL_E() and OVL_E_FLAGS() accessors

Just commit 5522c9c7cbd2 has conflict caused by
4609e1f18e19c ("fs: port ->permission() to pass mnt_idmap").
It is enough to change mnt_idmap() call to mnt_user_ns(),
in the rejected hunk.

Hi Andrey,

I don't have time to review this backport series, but in my memory,
these few commits were part of a much larger refactoring and
I am almost positive that there are fix commits that mention
Fixes: 0af950f57fef ("ovl: move ovl_entry into ovl_inode") in upstream kernel.

If you do want to backport overlayfs changes to stable kernels, you should
specify how you tested them and that should include at least running
the latest fstests overlayfs tests.

Thanks,
Amir.

Hi Amir,

I gave up the idea of making backports series of upstream patches, I just did what Hillf Danton suggested here:

https://groups.google.com/g/syzkaller-bugs/c/xDcxFKSppfE/m/b38Tv7LoAAAJ .

This looks like a more universal solution, suitable for previous kernel versions, especially since the likelihood of such an error occurring is quite low.

I did regression testing with xfstests-dev + unionmount-testsuite on version 5.10.208 and found no additional failures.

Please take a look at the attached patch.

Thanks,
Andrey. From: Andrey Kalachev <kalachev@xxxxxxxxx>
Date: Fri, 19 Apr 2024 20:55:10 +0300
Subject: [PATCH] ovl: fix general protection fault in security_inode_getattr

general protection fault caused by dentry shared between two processes.
Suppose we have a regular file `A` onto lower overlayfs layer, metacopy=on.

Process P1 execute link syscall ( `A` link to `B`), P2 do open `B`.

P1 | P2
-----------+--------------
sys_link |
| sys_open
| ovl_lookup B (1)
| ovl_alloc_entry (2)
|
ovl_link | (3)
|
|
sys_link | (4)
|
| ovl_open B (5)
| ovl_copy_up_one
| ovl_path_lower
| ovl_numlower(oe) (6)
| ovl_path_lower (7)
| vfs_getattr(struct path, ..)
| security_inode_getattr(struct path, ...)
| d_backing_inode(path->dentry)

(1) P2 call ovl_lookup B, lookup non existent file `B`,
new `B` dentry allocated.
(2) P2 call ovl_alloc_entry() in `non existent file` context,
return zero filled ovl_entry.
(3) P1 make hardlink A -> B,
now `B` dentry associated with lower layer file `A`.
(4) P1 return to userspace from `sys_link`,
leave ovl_entry `B` unchanged (zeros).
(5) P2 continue open file behind dentry `B`.
(6) P2 ovl_numlower(oe) return 0,
taken from zero filled ovl_entry `oe`
(7) P2 ovl_path_lower() return zero filled `struct path`, due numlower=0.
(8) P2 NULL dereference in d_backing_inode()

Since release v6.5 `ovl_entry` associated with inode and placed into
the `struct ovl_inode_params`.

Issue fixed at:
commit 0af950f57fef ("ovl: move ovl_entry into ovl_inode")

Solution, proposed by Hillf Danton, most common and suitable for
stable kernels before v6.5. The patch code is based on it.

Reported-by: syzbot+f07cc9be8d1d226947ed@xxxxxxxxxxxxxxxxxxxxxxxxx
Link: https://lore.kernel.org/lkml/0000000000008caae305ab9a5318@xxxxxxxxxx
Link: https://syzkaller.appspot.com/bug?extid=f07cc9be8d1d226947ed
Signed-off-by: Andrey Kalachev <kalachev@xxxxxxxxx>
---
fs/overlayfs/copy_up.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 65ac504595ba..b7352c43b673 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -879,6 +879,13 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
return -EROFS;

ovl_path_lower(dentry, &ctx.lowerpath);
+
+ if (unlikely(!ctx.lowerpath.dentry)) {
+ /* syzbot+f07cc9be8d1d226947ed@xxxxxxxxxxxxxxxxxxxxxxxxx */
+ pr_err("prevention GPF in security_inode_getattr()\n");
+ return -EIO;
+ }
+
err = vfs_getattr(&ctx.lowerpath, &ctx.stat,
STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
if (err)
--
2.30.2

Next message: Kirill A. Shutemov: "[PATCHv2] x86/tdx: Preserve shared bit on mprotect()"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v2 05/13] x86/irq: Reserve a per CPU IDT vector for posted MSIs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]