* WARNING: kmalloc bug in str_read
@ 2018-09-07 12:48 syzbot
2018-09-07 16:42 ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa
2018-09-07 21:24 ` WARNING: kmalloc bug in str_read syzbot
0 siblings, 2 replies; 16+ messages in thread
From: syzbot @ 2018-09-07 12:48 UTC (permalink / raw)
To: eparis, linux-kernel, paul, peter.enderborg, sds, selinux,
syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: ca16eb342ebe Merge tag 'for-linus-20180906' of git://git.k..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14cad421400000
kernel config: https://syzkaller.appspot.com/x/.config?x=6c9564cd177daf0c
dashboard link: https://syzkaller.appspot.com/bug?extid=ac488b9811036cea7ea0
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com
SELinux: policydb version 983061 does not match my version range 15-31
SELinux: ebitmap: truncated map
Unknown ioctl 1075883590
Unknown ioctl 1075883590
WARNING: CPU: 1 PID: 7505 at mm/slab_common.c:1031 kmalloc_slab+0x56/0x70
mm/slab_common.c:1031
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 7505 Comm: syz-executor7 Not tainted 4.19.0-rc2+ #4
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
panic+0x238/0x4e7 kernel/panic.c:184
__warn.cold.8+0x163/0x1ba kernel/panic.c:536
report_bug+0x252/0x2d0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:178 [inline]
do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:993
RIP: 0010:kmalloc_slab+0x56/0x70 mm/slab_common.c:1031
Code: c5 40 db f2 87 5d c3 b8 10 00 00 00 48 85 ff 74 f4 83 ef 01 c1 ef 03
0f b6 87 60 da f2 87 eb d8 31 c0 81 e6 00 02 00 00 75 db <0f> 0b 5d c3 48
8b 04 c5 80 da f2 87 5d c3 66 90 66 2e 0f 1f 84 00
RSP: 0018:ffff88018540f280 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 00000000299a7ba5 RCX: ffffc900024eb000
RDX: 00000000000018a3 RSI: 0000000000000000 RDI: 00000000299a7ba6
RBP: ffff88018540f280 R08: ffff88018958a080 R09: ffffed003b6246de
R10: ffffed003b6246de R11: ffff8801db1236f3 R12: 00000000006000c0
R13: ffff88018540f938 R14: ffff88018540f3c8 R15: 00000000006000c0
__do_kmalloc mm/slab.c:3713 [inline]
__kmalloc+0x25/0x720 mm/slab.c:3727
kmalloc include/linux/slab.h:518 [inline]
str_read+0x48/0x160 security/selinux/ss/policydb.c:1104
class_read+0x4a1/0xde0 security/selinux/ss/policydb.c:1345
policydb_read+0xf09/0x5f90 security/selinux/ss/policydb.c:2407
security_load_policy+0x23b/0x1650 security/selinux/ss/services.c:2165
sel_write_load+0x247/0x460 security/selinux/selinuxfs.c:565
__vfs_write+0x117/0x9d0 fs/read_write.c:485
vfs_write+0x1fc/0x560 fs/read_write.c:549
ksys_write+0x101/0x260 fs/read_write.c:598
__do_sys_write fs/read_write.c:610 [inline]
__se_sys_write fs/read_write.c:607 [inline]
__x64_sys_write+0x73/0xb0 fs/read_write.c:607
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x457099
Code: fd b4 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 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fc5edd57c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fc5edd586d4 RCX: 0000000000457099
RDX: 0000000000000094 RSI: 0000000020000000 RDI: 0000000000000003
RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000004d83b8 R14: 00000000004cae4e R15: 0000000000000000
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug 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@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-07 12:48 WARNING: kmalloc bug in str_read syzbot @ 2018-09-07 16:42 ` Tetsuo Handa 2018-09-13 3:02 ` Paul Moore 2018-09-07 21:24 ` WARNING: kmalloc bug in str_read syzbot 1 sibling, 1 reply; 16+ messages in thread From: Tetsuo Handa @ 2018-09-07 16:42 UTC (permalink / raw) To: selinux Cc: syzbot, eparis, linux-kernel, paul, peter.enderborg, sds, syzkaller-bugs syzbot is hitting warning at str_read() [1] because len parameter can become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for this case. [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> --- security/selinux/ss/policydb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index e9394e7..f4eadd3 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) if ((len == 0) || (len == (u32)-1)) return -EINVAL; - str = kmalloc(len + 1, flags); + str = kmalloc(len + 1, flags | __GFP_NOWARN); if (!str) return -ENOMEM; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-07 16:42 ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa @ 2018-09-13 3:02 ` Paul Moore 2018-09-13 6:26 ` Tetsuo Handa 2018-09-13 8:01 ` Dmitry Vyukov 0 siblings, 2 replies; 16+ messages in thread From: Paul Moore @ 2018-09-13 3:02 UTC (permalink / raw) To: penguin-kernel Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, peter.enderborg, Stephen Smalley, syzkaller-bugs On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > syzbot is hitting warning at str_read() [1] because len parameter can > become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > this case. > > [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > --- > security/selinux/ss/policydb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index e9394e7..f4eadd3 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > if ((len == 0) || (len == (u32)-1)) > return -EINVAL; > > - str = kmalloc(len + 1, flags); > + str = kmalloc(len + 1, flags | __GFP_NOWARN); > if (!str) > return -ENOMEM; Thanks for the patch. My eyes are starting to glaze over a bit chasing down all of the different kmalloc() code paths trying to ensure that this always does the right thing based on size of the allocation and the different slab allocators ... are we sure that this will always return NULL when (len + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator configurations? -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 3:02 ` Paul Moore @ 2018-09-13 6:26 ` Tetsuo Handa 2018-09-13 7:12 ` peter enderborg 2018-09-13 19:23 ` Paul Moore 2018-09-13 8:01 ` Dmitry Vyukov 1 sibling, 2 replies; 16+ messages in thread From: Tetsuo Handa @ 2018-09-13 6:26 UTC (permalink / raw) To: Paul Moore Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, peter.enderborg, Stephen Smalley, syzkaller-bugs, linux-mm On 2018/09/13 12:02, Paul Moore wrote: > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> syzbot is hitting warning at str_read() [1] because len parameter can >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >> this case. >> >> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >> --- >> security/selinux/ss/policydb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >> index e9394e7..f4eadd3 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >> if ((len == 0) || (len == (u32)-1)) >> return -EINVAL; >> >> - str = kmalloc(len + 1, flags); >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >> if (!str) >> return -ENOMEM; > > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return ZERO_SIZE_PTR) due to (len == (u32)-1) check above. The only concern would be whether you want allocation failure messages. I assumed you don't need it because we are returning -ENOMEM to the caller. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 6:26 ` Tetsuo Handa @ 2018-09-13 7:12 ` peter enderborg 2018-09-13 19:23 ` Paul Moore 1 sibling, 0 replies; 16+ messages in thread From: peter enderborg @ 2018-09-13 7:12 UTC (permalink / raw) To: Tetsuo Handa, Paul Moore Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > On 2018/09/13 12:02, Paul Moore wrote: >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> syzbot is hitting warning at str_read() [1] because len parameter can >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>> this case. >>> >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >>> --- >>> security/selinux/ss/policydb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>> index e9394e7..f4eadd3 100644 >>> --- a/security/selinux/ss/policydb.c >>> +++ b/security/selinux/ss/policydb.c >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >>> if ((len == 0) || (len == (u32)-1)) >>> return -EINVAL; >>> >>> - str = kmalloc(len + 1, flags); >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>> if (!str) >>> return -ENOMEM; >> Thanks for the patch. >> >> My eyes are starting to glaze over a bit chasing down all of the >> different kmalloc() code paths trying to ensure that this always does >> the right thing based on size of the allocation and the different slab >> allocators ... are we sure that this will always return NULL when (len >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >> configurations? >> > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. > Would it not be better with char *str; if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) return -EINVAL; str = kmalloc(len + 1, flags); if (!str) return -ENOMEM; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() @ 2018-09-13 7:12 ` peter enderborg 0 siblings, 0 replies; 16+ messages in thread From: peter enderborg @ 2018-09-13 7:12 UTC (permalink / raw) To: Tetsuo Handa, Paul Moore Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > On 2018/09/13 12:02, Paul Moore wrote: >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> syzbot is hitting warning at str_read() [1] because len parameter can >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>> this case. >>> >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >>> --- >>> security/selinux/ss/policydb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>> index e9394e7..f4eadd3 100644 >>> --- a/security/selinux/ss/policydb.c >>> +++ b/security/selinux/ss/policydb.c >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >>> if ((len == 0) || (len == (u32)-1)) >>> return -EINVAL; >>> >>> - str = kmalloc(len + 1, flags); >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>> if (!str) >>> return -ENOMEM; >> Thanks for the patch. >> >> My eyes are starting to glaze over a bit chasing down all of the >> different kmalloc() code paths trying to ensure that this always does >> the right thing based on size of the allocation and the different slab >> allocators ... are we sure that this will always return NULL when (len >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >> configurations? >> > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. > Would it not be better with char *str; if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) return -EINVAL; str = kmalloc(len + 1, flags); if (!str) return -ENOMEM; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 7:12 ` peter enderborg @ 2018-09-13 11:11 ` Michal Hocko -1 siblings, 0 replies; 16+ messages in thread From: Michal Hocko @ 2018-09-13 11:11 UTC (permalink / raw) To: peter enderborg Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On Thu 13-09-18 09:12:04, peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; I strongly suspect that you want kvmalloc rather than kmalloc here. The larger the request the more likely is the allocation to fail. I am not familiar with the code but I assume this is a root only interface so we don't have to worry about nasty users scenario. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() @ 2018-09-13 11:11 ` Michal Hocko 0 siblings, 0 replies; 16+ messages in thread From: Michal Hocko @ 2018-09-13 11:11 UTC (permalink / raw) To: peter enderborg Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On Thu 13-09-18 09:12:04, peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; I strongly suspect that you want kvmalloc rather than kmalloc here. The larger the request the more likely is the allocation to fail. I am not familiar with the code but I assume this is a root only interface so we don't have to worry about nasty users scenario. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 11:11 ` Michal Hocko @ 2018-09-13 12:55 ` peter enderborg -1 siblings, 0 replies; 16+ messages in thread From: peter enderborg @ 2018-09-13 12:55 UTC (permalink / raw) To: Michal Hocko Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On 09/13/2018 01:11 PM, Michal Hocko wrote: > On Thu 13-09-18 09:12:04, peter enderborg wrote: >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: >>> On 2018/09/13 12:02, Paul Moore wrote: >>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa >>>> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>>> syzbot is hitting warning at str_read() [1] because len parameter can >>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>>>> this case. >>>>> >>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>>>> >>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >>>>> --- >>>>> security/selinux/ss/policydb.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>>>> index e9394e7..f4eadd3 100644 >>>>> --- a/security/selinux/ss/policydb.c >>>>> +++ b/security/selinux/ss/policydb.c >>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >>>>> if ((len == 0) || (len == (u32)-1)) >>>>> return -EINVAL; >>>>> >>>>> - str = kmalloc(len + 1, flags); >>>>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>>>> if (!str) >>>>> return -ENOMEM; >>>> Thanks for the patch. >>>> >>>> My eyes are starting to glaze over a bit chasing down all of the >>>> different kmalloc() code paths trying to ensure that this always does >>>> the right thing based on size of the allocation and the different slab >>>> allocators ... are we sure that this will always return NULL when (len >>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >>>> configurations? >>>> >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. >>> >>> The only concern would be whether you want allocation failure messages. >>> I assumed you don't need it because we are returning -ENOMEM to the caller. >>> >> Would it not be better with >> >> char *str; >> >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) >> return -EINVAL; >> >> str = kmalloc(len + 1, flags); >> if (!str) >> return -ENOMEM; > I strongly suspect that you want kvmalloc rather than kmalloc here. The > larger the request the more likely is the allocation to fail. > > I am not familiar with the code but I assume this is a root only > interface so we don't have to worry about nasty users scenario. > I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option. And some of the calls are GFP_ATOMC. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() @ 2018-09-13 12:55 ` peter enderborg 0 siblings, 0 replies; 16+ messages in thread From: peter enderborg @ 2018-09-13 12:55 UTC (permalink / raw) To: Michal Hocko Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On 09/13/2018 01:11 PM, Michal Hocko wrote: > On Thu 13-09-18 09:12:04, peter enderborg wrote: >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: >>> On 2018/09/13 12:02, Paul Moore wrote: >>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa >>>> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>>> syzbot is hitting warning at str_read() [1] because len parameter can >>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>>>> this case. >>>>> >>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>>>> >>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >>>>> --- >>>>> security/selinux/ss/policydb.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>>>> index e9394e7..f4eadd3 100644 >>>>> --- a/security/selinux/ss/policydb.c >>>>> +++ b/security/selinux/ss/policydb.c >>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >>>>> if ((len == 0) || (len == (u32)-1)) >>>>> return -EINVAL; >>>>> >>>>> - str = kmalloc(len + 1, flags); >>>>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>>>> if (!str) >>>>> return -ENOMEM; >>>> Thanks for the patch. >>>> >>>> My eyes are starting to glaze over a bit chasing down all of the >>>> different kmalloc() code paths trying to ensure that this always does >>>> the right thing based on size of the allocation and the different slab >>>> allocators ... are we sure that this will always return NULL when (len >>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >>>> configurations? >>>> >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. >>> >>> The only concern would be whether you want allocation failure messages. >>> I assumed you don't need it because we are returning -ENOMEM to the caller. >>> >> Would it not be better with >> >> char *str; >> >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) >> return -EINVAL; >> >> str = kmalloc(len + 1, flags); >> if (!str) >> return -ENOMEM; > I strongly suspect that you want kvmalloc rather than kmalloc here. The > larger the request the more likely is the allocation to fail. > > I am not familiar with the code but I assume this is a root only > interface so we don't have to worry about nasty users scenario. > I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option. And some of the calls are GFP_ATOMC. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 12:55 ` peter enderborg (?) @ 2018-09-13 13:03 ` Dmitry Vyukov -1 siblings, 0 replies; 16+ messages in thread From: Dmitry Vyukov @ 2018-09-13 13:03 UTC (permalink / raw) To: peter enderborg Cc: Michal Hocko, Tetsuo Handa, Paul Moore, SELinux, syzbot+ac488b9811036cea7ea0, Eric Paris, LKML, Stephen Smalley, syzkaller-bugs, linux-mm On Thu, Sep 13, 2018 at 2:55 PM, peter enderborg <peter.enderborg@sony.com> wrote: >>>>>> syzbot is hitting warning at str_read() [1] because len parameter can >>>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >>>>>> this case. >>>>>> >>>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >>>>>> >>>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >>>>>> --- >>>>>> security/selinux/ss/policydb.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>>>>> index e9394e7..f4eadd3 100644 >>>>>> --- a/security/selinux/ss/policydb.c >>>>>> +++ b/security/selinux/ss/policydb.c >>>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >>>>>> if ((len == 0) || (len == (u32)-1)) >>>>>> return -EINVAL; >>>>>> >>>>>> - str = kmalloc(len + 1, flags); >>>>>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >>>>>> if (!str) >>>>>> return -ENOMEM; >>>>> Thanks for the patch. >>>>> >>>>> My eyes are starting to glaze over a bit chasing down all of the >>>>> different kmalloc() code paths trying to ensure that this always does >>>>> the right thing based on size of the allocation and the different slab >>>>> allocators ... are we sure that this will always return NULL when (len >>>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator >>>>> configurations? >>>>> >>>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return >>>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. >>>> >>>> The only concern would be whether you want allocation failure messages. >>>> I assumed you don't need it because we are returning -ENOMEM to the caller. >>>> >>> Would it not be better with >>> >>> char *str; >>> >>> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) >>> return -EINVAL; >>> >>> str = kmalloc(len + 1, flags); >>> if (!str) >>> return -ENOMEM; >> I strongly suspect that you want kvmalloc rather than kmalloc here. The >> larger the request the more likely is the allocation to fail. >> >> I am not familiar with the code but I assume this is a root only >> interface so we don't have to worry about nasty users scenario. >> > I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option. > > And some of the calls are GFP_ATOMC. Then another option is to introduce reasonable application-specific limit and not rely on kmalloc-anything at all. We did this for some instances of this warning too. One advantage of it is that it prevents users from doing silly things (or maybe will discover bugs in user-space code better, why are they asking for megs here?). Another advantage is that what works on one version of kernel will continue to work on another version of kernel. Today it's possible that a policy works on one kernel with 4MB kmalloc limit, but breaks on another with 2MB limit. Ideally exact value of KMALLOC_MAX_SIZE does not affect anything in user-space. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 12:55 ` peter enderborg (?) (?) @ 2018-09-13 19:35 ` Paul Moore -1 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2018-09-13 19:35 UTC (permalink / raw) To: penguin-kernel, peter.enderborg Cc: mhocko, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On Thu, Sep 13, 2018 at 8:55 AM peter enderborg <peter.enderborg@sony.com> wrote: > On 09/13/2018 01:11 PM, Michal Hocko wrote: > > On Thu 13-09-18 09:12:04, peter enderborg wrote: > >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > >>> On 2018/09/13 12:02, Paul Moore wrote: > >>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >>>> <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>>>> syzbot is hitting warning at str_read() [1] because len parameter can > >>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>>>> this case. > >>>>> > >>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>>>> > >>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > >>>>> --- > >>>>> security/selinux/ss/policydb.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >>>>> index e9394e7..f4eadd3 100644 > >>>>> --- a/security/selinux/ss/policydb.c > >>>>> +++ b/security/selinux/ss/policydb.c > >>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > >>>>> if ((len == 0) || (len == (u32)-1)) > >>>>> return -EINVAL; > >>>>> > >>>>> - str = kmalloc(len + 1, flags); > >>>>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>>>> if (!str) > >>>>> return -ENOMEM; > >>>> Thanks for the patch. > >>>> > >>>> My eyes are starting to glaze over a bit chasing down all of the > >>>> different kmalloc() code paths trying to ensure that this always does > >>>> the right thing based on size of the allocation and the different slab > >>>> allocators ... are we sure that this will always return NULL when (len > >>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >>>> configurations? > >>>> > >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > >>> > >>> The only concern would be whether you want allocation failure messages. > >>> I assumed you don't need it because we are returning -ENOMEM to the caller. > >>> > >> Would it not be better with > >> > >> char *str; > >> > >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > >> return -EINVAL; > >> > >> str = kmalloc(len + 1, flags); > >> if (!str) > >> return -ENOMEM; > > I strongly suspect that you want kvmalloc rather than kmalloc here. The > > larger the request the more likely is the allocation to fail. > > > > I am not familiar with the code but I assume this is a root only > > interface so we don't have to worry about nasty users scenario. > > > I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option. > > And some of the calls are GFP_ATOMC. Based on all the comments it looks like Tetsuo's original patch is probably the best fix right now. I'm going to merge this into selinux/next. Tetsuo, thanks for the patch, and thanks to everyone else for the comments/review. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 7:12 ` peter enderborg (?) (?) @ 2018-09-13 19:28 ` Paul Moore -1 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2018-09-13 19:28 UTC (permalink / raw) To: peter.enderborg Cc: penguin-kernel, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm On Thu, Sep 13, 2018 at 3:12 AM peter enderborg <peter.enderborg@sony.com> wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; As long as it's safe, I'd rather leave the maximum allocation limit as a kmalloc internal and let kmalloc return NULL if we try too large of an allocation. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 6:26 ` Tetsuo Handa 2018-09-13 7:12 ` peter enderborg @ 2018-09-13 19:23 ` Paul Moore 1 sibling, 0 replies; 16+ messages in thread From: Paul Moore @ 2018-09-13 19:23 UTC (permalink / raw) To: penguin-kernel Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel, peter.enderborg, Stephen Smalley, syzkaller-bugs, linux-mm On Thu, Sep 13, 2018 at 2:26 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2018/09/13 12:02, Paul Moore wrote: > > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > > <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> syzbot is hitting warning at str_read() [1] because len parameter can > >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >> this case. > >> > >> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >> > >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> > >> --- > >> security/selinux/ss/policydb.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >> index e9394e7..f4eadd3 100644 > >> --- a/security/selinux/ss/policydb.c > >> +++ b/security/selinux/ss/policydb.c > >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) > >> if ((len == 0) || (len == (u32)-1)) > >> return -EINVAL; > >> > >> - str = kmalloc(len + 1, flags); > >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >> if (!str) > >> return -ENOMEM; > > > > Thanks for the patch. > > > > My eyes are starting to glaze over a bit chasing down all of the > > different kmalloc() code paths trying to ensure that this always does > > the right thing based on size of the allocation and the different slab > > allocators ... are we sure that this will always return NULL when (len > > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > > configurations? > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. I'm not to worried about the failure messages, returning -ENOMEM should be sufficient in this case. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() 2018-09-13 3:02 ` Paul Moore 2018-09-13 6:26 ` Tetsuo Handa @ 2018-09-13 8:01 ` Dmitry Vyukov 1 sibling, 0 replies; 16+ messages in thread From: Dmitry Vyukov @ 2018-09-13 8:01 UTC (permalink / raw) To: Paul Moore Cc: Tetsuo Handa, SELinux, syzbot+ac488b9811036cea7ea0, Eric Paris, LKML, peter.enderborg, Stephen Smalley, syzkaller-bugs On Thu, Sep 13, 2018 at 5:02 AM, Paul Moore <paul@paul-moore.com> wrote: > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> syzbot is hitting warning at str_read() [1] because len parameter can >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for >> this case. >> >> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com> >> --- >> security/selinux/ss/policydb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >> index e9394e7..f4eadd3 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len) >> if ((len == 0) || (len == (u32)-1)) >> return -EINVAL; >> >> - str = kmalloc(len + 1, flags); >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); >> if (!str) >> return -ENOMEM; > > Thanks for the patch. > > My eyes are starting to glaze over a bit chasing down all of the > different kmalloc() code paths trying to ensure that this always does > the right thing based on size of the allocation and the different slab > allocators ... are we sure that this will always return NULL when (len > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > configurations? Yes, it's the blessed way to do it. We have lots of similar cases: https://elixir.bootlin.com/linux/v4.19-rc3/ident/__GFP_NOWARN ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: WARNING: kmalloc bug in str_read 2018-09-07 12:48 WARNING: kmalloc bug in str_read syzbot 2018-09-07 16:42 ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa @ 2018-09-07 21:24 ` syzbot 1 sibling, 0 replies; 16+ messages in thread From: syzbot @ 2018-09-07 21:24 UTC (permalink / raw) To: eparis, linux-kernel, paul, penguin-kernel, peter.enderborg, sds, selinux, syzkaller-bugs syzbot has found a reproducer for the following crash on: HEAD commit: a49a9dcce802 Merge tag 'drm-fixes-2018-09-07' of git://ano.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=11981149400000 kernel config: https://syzkaller.appspot.com/x/.config?x=6c9564cd177daf0c dashboard link: https://syzkaller.appspot.com/bug?extid=ac488b9811036cea7ea0 compiler: gcc (GCC) 8.0.1 20180413 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1717dfa9400000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13c1adea400000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com random: sshd: uninitialized urandom read (32 bytes read) random: sshd: uninitialized urandom read (32 bytes read) random: sshd: uninitialized urandom read (32 bytes read) random: sshd: uninitialized urandom read (32 bytes read) audit: type=1400 audit(1536355256.676:7): avc: denied { map } for pid=4365 comm="syz-executor462" path="/root/syz-executor462675731" dev="sda1" ino=16481 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023 tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1 WARNING: CPU: 0 PID: 4365 at mm/slab_common.c:1031 kmalloc_slab+0x56/0x70 mm/slab_common.c:1031 Kernel panic - not syncing: panic_on_warn set ... CPU: 0 PID: 4365 Comm: syz-executor462 Not tainted 4.19.0-rc2+ #5 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113 panic+0x238/0x4e7 kernel/panic.c:184 __warn.cold.8+0x163/0x1ba kernel/panic.c:536 report_bug+0x252/0x2d0 lib/bug.c:186 fixup_bug arch/x86/kernel/traps.c:178 [inline] do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316 invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:993 RIP: 0010:kmalloc_slab+0x56/0x70 mm/slab_common.c:1031 Code: c5 40 db f2 87 5d c3 b8 10 00 00 00 48 85 ff 74 f4 83 ef 01 c1 ef 03 0f b6 87 60 da f2 87 eb d8 31 c0 81 e6 00 02 00 00 75 db <0f> 0b 5d c3 48 8b 04 c5 80 da f2 87 5d c3 66 90 66 2e 0f 1f 84 00 RSP: 0018:ffff8801c2317298 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 000000000d7fffd6 RCX: ffffffff832e9d2e RDX: 0000000000000000 RSI: 0000000000000000 RDI: 000000000d7fffd7 RBP: ffff8801c2317298 R08: ffff8801c197a700 R09: ffffed003b6046de R10: ffffed003b6046de R11: ffff8801db0236f3 R12: 00000000006000c0 R13: ffff8801c2317938 R14: ffff8801c23173c0 R15: 00000000006000c0 __do_kmalloc mm/slab.c:3713 [inline] __kmalloc+0x25/0x720 mm/slab.c:3727 kmalloc include/linux/slab.h:518 [inline] str_read+0x48/0x160 security/selinux/ss/policydb.c:1104 common_read+0x37c/0x560 security/selinux/ss/policydb.c:1177 policydb_read+0xf09/0x5f90 security/selinux/ss/policydb.c:2407 security_load_policy+0x23b/0x1650 security/selinux/ss/services.c:2165 sel_write_load+0x247/0x460 security/selinux/selinuxfs.c:565 __vfs_write+0x117/0x9d0 fs/read_write.c:485 vfs_write+0x1fc/0x560 fs/read_write.c:549 ksys_write+0x101/0x260 fs/read_write.c:598 __do_sys_write fs/read_write.c:610 [inline] __se_sys_write fs/read_write.c:607 [inline] __x64_sys_write+0x73/0xb0 fs/read_write.c:607 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x440049 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 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 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffd03e23ca8 EFLAGS: 00000213 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440049 RDX: 0000000000000163 RSI: 0000000020000380 RDI: 0000000000000003 RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8 R10: 0000000000000000 R11: 0000000000000213 R12: 00000000004018d0 R13: 0000000000401960 R14: 0000000000000000 R15: 0000000000000000 Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled Rebooting in 86400 seconds.. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-09-13 19:35 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-07 12:48 WARNING: kmalloc bug in str_read syzbot 2018-09-07 16:42 ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa 2018-09-13 3:02 ` Paul Moore 2018-09-13 6:26 ` Tetsuo Handa 2018-09-13 7:12 ` peter enderborg 2018-09-13 7:12 ` peter enderborg 2018-09-13 11:11 ` Michal Hocko 2018-09-13 11:11 ` Michal Hocko 2018-09-13 12:55 ` peter enderborg 2018-09-13 12:55 ` peter enderborg 2018-09-13 13:03 ` Dmitry Vyukov 2018-09-13 19:35 ` Paul Moore 2018-09-13 19:28 ` Paul Moore 2018-09-13 19:23 ` Paul Moore 2018-09-13 8:01 ` Dmitry Vyukov 2018-09-07 21:24 ` WARNING: kmalloc bug in str_read syzbot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.