[syzbot] [lsm?] WARNING in handle_policy_update

[syzbot] [lsm?] WARNING in handle_policy_update

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f932fb9b4074 Merge tag 'v6.13-rc2-ksmbd-server-fixes' of g..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10892d44580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=99a5586995ec03b2
dashboard link: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1696ea0f980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16509cdf980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/f0d0c95f5364/disk-f932fb9b.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/201cf3c7a7b5/vmlinux-f932fb9b.xz
kernel image: https://storage.googleapis.com/syzbot-assets/fcb972084579/bzImage-f932fb9b.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com

------------[ cut here ]------------
WARNING: CPU: 0 PID: 5827 at mm/page_alloc.c:4727 __alloc_pages_noprof+0xeff/0x25b0 mm/page_alloc.c:4727
Modules linked in:
CPU: 0 UID: 0 PID: 5827 Comm: syz-executor752 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
RIP: 0010:__alloc_pages_noprof+0xeff/0x25b0 mm/page_alloc.c:4727
Code: 24 2c 00 00 00 00 89 cd 0f 84 8b f9 ff ff 8b 34 24 48 89 da 8b 7c 24 08 e8 0e b3 fe ff e9 69 f9 ff ff c6 05 e1 44 16 0e 01 90 <0f> 0b 90 31 db e9 9f f3 ff ff 89 14 24 e8 9f a2 0c 00 8b 14 24 e9
RSP: 0018:ffffc900015978e8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000013 RDI: 0000000000040cc0
RBP: 0000000000000000 R08: 0000000000000005 R09: 0000000000000100
R10: 0000000000000100 R11: ffffffff81fb9344 R12: 0000000000000013
R13: 0000000000040cc0 R14: 1ffff920002b2f31 R15: 00000000ffffffff
FS:  000055556162e380(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000130 CR3: 0000000028cc2000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __alloc_pages_node_noprof include/linux/gfp.h:269 [inline]
 alloc_pages_node_noprof include/linux/gfp.h:296 [inline]
 ___kmalloc_large_node+0x84/0x1b0 mm/slub.c:4228
 __kmalloc_large_node_noprof+0x1c/0x70 mm/slub.c:4255
 __do_kmalloc_node mm/slub.c:4271 [inline]
 __kmalloc_node_track_caller_noprof.cold+0x5/0x5f mm/slub.c:4302
 memdup_user_nul+0x2b/0x110 mm/util.c:305
 handle_policy_update+0x188/0x11e0 security/safesetid/securityfs.c:153
 safesetid_gid_file_write+0x87/0xc0 security/safesetid/securityfs.c:260
 vfs_write+0x24c/0x1150 fs/read_write.c:677
 ksys_write+0x12b/0x250 fs/read_write.c:731
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f8b2d0d02e9
Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 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 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd58b75818 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007ffd58b759f8 RCX: 00007f8b2d0d02e9
RDX: 00000000fffffdef RSI: 0000000000000000 RDI: 0000000000000003
RBP: 00007f8b2d143610 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffd58b759e8 R14: 0000000000000001 R15: 0000000000000001
 </TASK>


---
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@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[PATCH] lsm: check size of writes

[Leo Stone]

syzbot attempts to write a buffer with a large size to a sysfs entry
with writes handled by safesetid_gid_file_write(), triggering a warning
in kmalloc.

Check the size specified for write buffers before allocating.

Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
Signed-off-by: Leo Stone <leocstone@gmail.com>
---
 security/safesetid/securityfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 25310468bcdd..5eba4c7f8d9e 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -254,7 +254,7 @@ static ssize_t safesetid_gid_file_write(struct file *file,
 	if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
 		return -EPERM;
 
-	if (*ppos != 0)
+	if (*ppos != 0 || len >= KMALLOC_MAX_SIZE)
 		return -EINVAL;
 
 	return handle_policy_update(file, buf, len, GID);
-- 
2.43.0

Re: [PATCH] lsm: check size of writes

[Tetsuo Handa]

On 2024/12/16 12:02, Leo Stone wrote:
> syzbot attempts to write a buffer with a large size to a sysfs entry
> with writes handled by safesetid_gid_file_write(), triggering a warning
> in kmalloc.
> 
> Check the size specified for write buffers before allocating.
> 
> Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> Signed-off-by: Leo Stone <leocstone@gmail.com>

Since handle_policy_update() has two callers, you should check
inside handle_policy_update().

By the way, since syzbot found this pattern in multiple LSM modules,
do we want to ask Andrew Morton to send all patches in one pull request
(instead of sending trivial patch from multiple LSM modules) ?

> ---
>  security/safesetid/securityfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 25310468bcdd..5eba4c7f8d9e 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -254,7 +254,7 @@ static ssize_t safesetid_gid_file_write(struct file *file,
>  	if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
>  		return -EPERM;
>  
> -	if (*ppos != 0)
> +	if (*ppos != 0 || len >= KMALLOC_MAX_SIZE)
>  		return -EINVAL;
>  
>  	return handle_policy_update(file, buf, len, GID);

Re: [PATCH] lsm: check size of writes

[Paul Moore]

On Mon, Dec 16, 2024 at 4:59 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2024/12/16 12:02, Leo Stone wrote:
> > syzbot attempts to write a buffer with a large size to a sysfs entry
> > with writes handled by safesetid_gid_file_write(), triggering a warning
> > in kmalloc.
> >
> > Check the size specified for write buffers before allocating.
> >
> > Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> > Signed-off-by: Leo Stone <leocstone@gmail.com>
>
> Since handle_policy_update() has two callers, you should check
> inside handle_policy_update().
>
> By the way, since syzbot found this pattern in multiple LSM modules,
> do we want to ask Andrew Morton to send all patches in one pull request
> (instead of sending trivial patch from multiple LSM modules) ?

I think you mean Micah Morton (mortonm) and not Andrew Morton (akpm),
yes?  Micah is the SafeSetID maintainer while Andrew maintains the
memory subsystem, and likely a few others.

In order to help prevent merge conflicts across the different LSM
trees I think it would be best for each LSM maintainer to merge their
respective patches.  If one of the maintainers is not responding after
a period of time, I can pick up the patch via the LSM tree.  We have
had some issues with SafeSetID in the past, but I'm hopeful we
resolved that with Micah and we just need to give him some time to
review and pickup this patch.

> > ---
> >  security/safesetid/securityfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 25310468bcdd..5eba4c7f8d9e 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -254,7 +254,7 @@ static ssize_t safesetid_gid_file_write(struct file *file,
> >       if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
> >               return -EPERM;
> >
> > -     if (*ppos != 0)
> > +     if (*ppos != 0 || len >= KMALLOC_MAX_SIZE)
> >               return -EINVAL;
> >
> >       return handle_policy_update(file, buf, len, GID);

-- 
paul-moore.com

[PATCH v2] lsm: check size of writes

[Leo Stone]

syzbot attempts to write a buffer with a large size to a sysfs entry
with writes handled by handle_policy_update(), triggering a warning
in kmalloc.

Check the size specified for write buffers before allocating.

Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
Signed-off-by: Leo Stone <leocstone@gmail.com>
---
v2: Make the check in handle_policy_update() to also cover
safesetid_uid_file_write(). Thanks for your feedback.
v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
---
 security/safesetid/securityfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 25310468bcdd..8e1ffd70b18a 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -143,6 +143,9 @@ static ssize_t handle_policy_update(struct file *file,
 	char *buf, *p, *end;
 	int err;
 
+	if (len >= KMALLOC_MAX_SIZE)
+		return -EINVAL;
+
 	pol = kmalloc(sizeof(struct setid_ruleset), GFP_KERNEL);
 	if (!pol)
 		return -ENOMEM;
-- 
2.43.0

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
>
> syzbot attempts to write a buffer with a large size to a sysfs entry
> with writes handled by handle_policy_update(), triggering a warning
> in kmalloc.
>
> Check the size specified for write buffers before allocating.
>
> Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> Signed-off-by: Leo Stone <leocstone@gmail.com>
> ---
> v2: Make the check in handle_policy_update() to also cover
> safesetid_uid_file_write(). Thanks for your feedback.
> v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> ---
>  security/safesetid/securityfs.c | 3 +++
>  1 file changed, 3 insertions(+)

Looks okay to me.  Micah, are you planning to merge this patch, or
would you like me to take it via the LSM tree?

Reviewed-by: Paul Moore <paul@paul-moore.com>

I'm going to tag this to come back to it in a week or so in case we
don't hear from Micah, but if you don't see any further replies Leo,
feel free to send a gentle nudge ;)

> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 25310468bcdd..8e1ffd70b18a 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -143,6 +143,9 @@ static ssize_t handle_policy_update(struct file *file,
>         char *buf, *p, *end;
>         int err;
>
> +       if (len >= KMALLOC_MAX_SIZE)
> +               return -EINVAL;
> +
>         pol = kmalloc(sizeof(struct setid_ruleset), GFP_KERNEL);
>         if (!pol)
>                 return -ENOMEM;
> --
> 2.43.0

--
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Tetsuo Handa]

On 2024/12/19 6:51, Paul Moore wrote:
> On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
>>
>> syzbot attempts to write a buffer with a large size to a sysfs entry
>> with writes handled by handle_policy_update(), triggering a warning
>> in kmalloc.
>>
>> Check the size specified for write buffers before allocating.
>>
>> Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
>> Signed-off-by: Leo Stone <leocstone@gmail.com>
>> ---
>> v2: Make the check in handle_policy_update() to also cover
>> safesetid_uid_file_write(). Thanks for your feedback.
>> v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
>> ---
>>  security/safesetid/securityfs.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> Looks okay to me.  Micah, are you planning to merge this patch, or
> would you like me to take it via the LSM tree?
> 
> Reviewed-by: Paul Moore <paul@paul-moore.com>
> 
> I'm going to tag this to come back to it in a week or so in case we
> don't hear from Micah, but if you don't see any further replies Leo,
> feel free to send a gentle nudge ;)

FYI: I sent

https://lkml.kernel.org/r/014cd694-cc27-4a07-a34a-2ae95d744515@I-love.SAKURA.ne.jp

which makes this patch redundant if my patch is accepted.

Re: [PATCH v2] lsm: check size of writes

[Tetsuo Handa]

Hello, Kees.

On 2024/12/21 19:00, Tetsuo Handa wrote:
> FYI: I sent
> 
> https://lkml.kernel.org/r/014cd694-cc27-4a07-a34a-2ae95d744515@I-love.SAKURA.ne.jp
> 
> which makes this patch redundant if my patch is accepted.
> 

I got a question regarding commit d73778e4b867 ("mm/util: Use dedicated
slab buckets for memdup_user()").

While I consider that using the same slab buckets for memdup_user() and
memdup_user_nul() is OK, I came to feel that we could utilize
kmem_buckets_create() more aggressively for debug purpose and/or
isolation purpose.

I got three bug reports on TOMOYO
https://lkml.kernel.org/r/67646895.050a0220.1dcc64.0023.GAE@google.com
and I guess that at least the fix for the first bug is
https://lkml.kernel.org/r/20241218185000.17920-2-leocstone@gmail.com
because the syz reproducer includes access to
/sys/kernel/config/nvmet/discovery_nqn interface.

If the slab buckets for nvmet and TOMOYO were separated, we might have
received these bug reports as nvmet bugs rather than TOMOYO bugs.

We switched to use module-local workqueue if that module needs to call
flush_workqueue() because calling flush_workqueue() against the kernel global
workqueues might introduce unpredictable locking dependency. Therefore, I came
to feel that it might be helpful to add a kernel config option for switching
whether to use dedicated slab buckets for individual module/subsystems.

For example, I don't know whether it is worth using a dedicated slab bucket
for each LSM module, but I feel that having a dedicated slab bucket shared
between all LSM modules might be worth doing, in order to reduce possibility
of by error overrunning into chunks used by LSM modules caused by bugs in
unrelated code.

Maybe we want a flag for not to bloat /proc/slabinfo output if we allow
using dedicated slab buckets for individual module/subsystems...

What do you think?

Re: [PATCH v2] lsm: check size of writes

[Kees Cook]

On Sat, Dec 21, 2024 at 10:40:45PM +0900, Tetsuo Handa wrote:
> Hello, Kees.
> 
> On 2024/12/21 19:00, Tetsuo Handa wrote:
> > FYI: I sent
> > 
> > https://lkml.kernel.org/r/014cd694-cc27-4a07-a34a-2ae95d744515@I-love.SAKURA.ne.jp
> > 
> > which makes this patch redundant if my patch is accepted.
> > 
> 
> I got a question regarding commit d73778e4b867 ("mm/util: Use dedicated
> slab buckets for memdup_user()").
> 
> While I consider that using the same slab buckets for memdup_user() and
> memdup_user_nul() is OK, I came to feel that we could utilize
> kmem_buckets_create() more aggressively for debug purpose and/or
> isolation purpose.

Sure!

> 
> I got three bug reports on TOMOYO
> https://lkml.kernel.org/r/67646895.050a0220.1dcc64.0023.GAE@google.com
> and I guess that at least the fix for the first bug is
> https://lkml.kernel.org/r/20241218185000.17920-2-leocstone@gmail.com
> because the syz reproducer includes access to
> /sys/kernel/config/nvmet/discovery_nqn interface.
> 
> If the slab buckets for nvmet and TOMOYO were separated, we might have
> received these bug reports as nvmet bugs rather than TOMOYO bugs.
> 
> We switched to use module-local workqueue if that module needs to call
> flush_workqueue() because calling flush_workqueue() against the kernel global
> workqueues might introduce unpredictable locking dependency. Therefore, I came
> to feel that it might be helpful to add a kernel config option for switching
> whether to use dedicated slab buckets for individual module/subsystems.
> 
> For example, I don't know whether it is worth using a dedicated slab bucket
> for each LSM module, but I feel that having a dedicated slab bucket shared
> between all LSM modules might be worth doing, in order to reduce possibility
> of by error overrunning into chunks used by LSM modules caused by bugs in
> unrelated code.

If the LSM core did a kmem_buckets_create() for each LSM, and the LSMs
were adjusted to explicitly allocate from their own bucket set, that
would be one way. Or just for the LSM as a whole (1 set of buckets
instead of a set for each LSM). I'd be happy to review patches for
either idea.

> Maybe we want a flag for not to bloat /proc/slabinfo output if we allow
> using dedicated slab buckets for individual module/subsystems...

No, I think accuracy for slabinfo is more important.

> What do you think?

I think per-site buckets is going to be the most effective long-term:
https://lore.kernel.org/lkml/20240809072532.work.266-kees@kernel.org/

But that doesn't exclude new kmem_buckets_create() users.

-Kees

-- 
Kees Cook

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Wed, Dec 18, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
> >
> > syzbot attempts to write a buffer with a large size to a sysfs entry
> > with writes handled by handle_policy_update(), triggering a warning
> > in kmalloc.
> >
> > Check the size specified for write buffers before allocating.
> >
> > Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> > Signed-off-by: Leo Stone <leocstone@gmail.com>
> > ---
> > v2: Make the check in handle_policy_update() to also cover
> > safesetid_uid_file_write(). Thanks for your feedback.
> > v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> > ---
> >  security/safesetid/securityfs.c | 3 +++
> >  1 file changed, 3 insertions(+)
>
> Looks okay to me.  Micah, are you planning to merge this patch, or
> would you like me to take it via the LSM tree?
>
> Reviewed-by: Paul Moore <paul@paul-moore.com>
>
> I'm going to tag this to come back to it in a week or so in case we
> don't hear from Micah, but if you don't see any further replies Leo,
> feel free to send a gentle nudge ;)

I'm going to go ahead and merge this into lsm/dev, if Micah has a
problem with that I can always remove/revert if needed.

We may need to revisit safesetid's documented support status, but
that's a topic for another day.

Thanks everyone.

-- 
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Sat, Dec 21, 2024 at 5:01 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2024/12/19 6:51, Paul Moore wrote:
> > On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
> >>
> >> syzbot attempts to write a buffer with a large size to a sysfs entry
> >> with writes handled by handle_policy_update(), triggering a warning
> >> in kmalloc.
> >>
> >> Check the size specified for write buffers before allocating.
> >>
> >> Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> >> Signed-off-by: Leo Stone <leocstone@gmail.com>
> >> ---
> >> v2: Make the check in handle_policy_update() to also cover
> >> safesetid_uid_file_write(). Thanks for your feedback.
> >> v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> >> ---
> >>  security/safesetid/securityfs.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >
> > Looks okay to me.  Micah, are you planning to merge this patch, or
> > would you like me to take it via the LSM tree?
> >
> > Reviewed-by: Paul Moore <paul@paul-moore.com>
> >
> > I'm going to tag this to come back to it in a week or so in case we
> > don't hear from Micah, but if you don't see any further replies Leo,
> > feel free to send a gentle nudge ;)
>
> FYI: I sent
>
> https://lkml.kernel.org/r/014cd694-cc27-4a07-a34a-2ae95d744515@I-love.SAKURA.ne.jp
>
> which makes this patch redundant if my patch is accepted.

Sure, but this patch is trivial, and there is no way the
KMALLOC_MAX_SIZE is limiting any normal use of safesetid so it seems
safe to apply now.  We can always revisit this change in the future
depending on how the rest of the kernel changes.

-- 
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Mon, Dec 23, 2024 at 12:33 AM Kees Cook <kees@kernel.org> wrote:
>
> If the LSM core did a kmem_buckets_create() for each LSM, and the LSMs
> were adjusted to explicitly allocate from their own bucket set, that
> would be one way. Or just for the LSM as a whole (1 set of buckets
> instead of a set for each LSM). I'd be happy to review patches for
> either idea.

If we're doing the work to shift over to kmem_buckets, it seems like
creating per-LSM buckets is the better option unless I'm missing
something.

I'm also not sure why the LSM framework would need to call
kmem_buckets_create() on behalf of the individual LSMs, can someone
help me understand why the individual LSMs couldn't do it in their
init routines?

If it is necessary for the LSM framework to create the buckets and
hand them back to the individual LSMs, I would suggest adding a new
flag to the lsm_info->flags field that a LSM could set to request a
kmem_bucket, and then add a new field to lsm_info that the LSM
framework could use to return the bucket to the LSM.  LSMs could
opt-in to kmem_buckets when they found the time to convert.

> I think per-site buckets is going to be the most effective long-term:
> https://lore.kernel.org/lkml/20240809072532.work.266-kees@kernel.org/
>
> But that doesn't exclude new kmem_buckets_create() users.

Is there an update on the per-site buckets?  I agree that would be the
preferable solution from a hardening perspective, and if it is on the
horizon it may not be worth the effort to convert the LSMs over to an
explicit kmem_buckets approach.

-- 
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Kees Cook]

On Sat, Jan 04, 2025 at 11:04:09PM -0500, Paul Moore wrote:
> On Mon, Dec 23, 2024 at 12:33 AM Kees Cook <kees@kernel.org> wrote:
> >
> > If the LSM core did a kmem_buckets_create() for each LSM, and the LSMs
> > were adjusted to explicitly allocate from their own bucket set, that
> > would be one way. Or just for the LSM as a whole (1 set of buckets
> > instead of a set for each LSM). I'd be happy to review patches for
> > either idea.
> 
> If we're doing the work to shift over to kmem_buckets, it seems like
> creating per-LSM buckets is the better option unless I'm missing
> something.
> 
> I'm also not sure why the LSM framework would need to call
> kmem_buckets_create() on behalf of the individual LSMs, can someone
> help me understand why the individual LSMs couldn't do it in their
> init routines?

When we moved stuff around for stacking, we moved other allocation
duties into the "core" of the LSM, so it just seemed reasonable to me to
do it again if this happened.

> If it is necessary for the LSM framework to create the buckets and
> hand them back to the individual LSMs, I would suggest adding a new
> flag to the lsm_info->flags field that a LSM could set to request a
> kmem_bucket, and then add a new field to lsm_info that the LSM
> framework could use to return the bucket to the LSM.  LSMs could
> opt-in to kmem_buckets when they found the time to convert.

Yeah, agreed. Since allocations would need to swap kmalloc() for
kmem_bucket_alloc(), we could also create something like lsm_alloc() and
hide everything from the individual LSMs -- the core would handle
allocating and using the buckets handle, etc.

Does anyone want to make a series for this? I am not planning to -- I'm
focused on the per-site implementation.

> > I think per-site buckets is going to be the most effective long-term:
> > https://lore.kernel.org/lkml/20240809072532.work.266-kees@kernel.org/
> >
> > But that doesn't exclude new kmem_buckets_create() users.
> 
> Is there an update on the per-site buckets?  I agree that would be the
> preferable solution from a hardening perspective, and if it is on the
> horizon it may not be worth the effort to convert the LSMs over to an
> explicit kmem_buckets approach.

I haven't had a chance to refresh the patch series, but the implementation
still works well. Besides some smaller feedback, I had also wanted to
make the individual buckets be allocated as needed. That way if something
was only doing allocations in, say, the 16 to 128 byte range, we wouldn't
lose memory to track the (unused) higher order bucket sizes.

I expect to send out the next revision after the coming merge window.

-Kees

-- 
Kees Cook

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Mon, Jan 6, 2025 at 7:09 PM Kees Cook <kees@kernel.org> wrote:
> On Sat, Jan 04, 2025 at 11:04:09PM -0500, Paul Moore wrote:
> > On Mon, Dec 23, 2024 at 12:33 AM Kees Cook <kees@kernel.org> wrote:
> > >
> > > If the LSM core did a kmem_buckets_create() for each LSM, and the LSMs
> > > were adjusted to explicitly allocate from their own bucket set, that
> > > would be one way. Or just for the LSM as a whole (1 set of buckets
> > > instead of a set for each LSM). I'd be happy to review patches for
> > > either idea.
> >
> > If we're doing the work to shift over to kmem_buckets, it seems like
> > creating per-LSM buckets is the better option unless I'm missing
> > something.
> >
> > I'm also not sure why the LSM framework would need to call
> > kmem_buckets_create() on behalf of the individual LSMs, can someone
> > help me understand why the individual LSMs couldn't do it in their
> > init routines?
>
> When we moved stuff around for stacking, we moved other allocation
> duties into the "core" of the LSM, so it just seemed reasonable to me to
> do it again if this happened.

Moving the allocation of the LSM state blobs for core kernel entities,
e.g. inode->i_security, was a bit different as it was necessary to
support multiple LSMs.  How could we support multiple simultaneous
LSMs if each of them wanted to control the inode->i_security field?

Moving arbitrary LSM allocations out of the individual LSMs and into
the LSM framework itself is definitely *not* something we want to do.
We really want to keep the LSM framework as small as possible.

> > If it is necessary for the LSM framework to create the buckets and
> > hand them back to the individual LSMs, I would suggest adding a new
> > flag to the lsm_info->flags field that a LSM could set to request a
> > kmem_bucket, and then add a new field to lsm_info that the LSM
> > framework could use to return the bucket to the LSM.  LSMs could
> > opt-in to kmem_buckets when they found the time to convert.
>
> Yeah, agreed. Since allocations would need to swap kmalloc() for
> kmem_bucket_alloc(), we could also create something like lsm_alloc() and
> hide everything from the individual LSMs -- the core would handle
> allocating and using the buckets handle, etc.
>
> Does anyone want to make a series for this? I am not planning to -- I'm
> focused on the per-site implementation.

I personally have enough other things to work on, and review, right
now that I don't see having any time to work on this in the near or
mid term, especially as a more preferred solution is in the works.
Putting on my SELinux maintainer hat for a moment, as long as there is
some belief that we'll have a per-site implementation, I'm not sure I
would merge a per-LSM bucket patchset right now as it would be work
that we would need to unroll once we had the per-site work upstream
... I would need to think about that.  Of course other LSM maintainers
might feel differently about their respective LSMs.

> > > I think per-site buckets is going to be the most effective long-term:
> > > https://lore.kernel.org/lkml/20240809072532.work.266-kees@kernel.org/
> > >
> > > But that doesn't exclude new kmem_buckets_create() users.
> >
> > Is there an update on the per-site buckets?  I agree that would be the
> > preferable solution from a hardening perspective, and if it is on the
> > horizon it may not be worth the effort to convert the LSMs over to an
> > explicit kmem_buckets approach.
>
> I haven't had a chance to refresh the patch series, but the implementation
> still works well. Besides some smaller feedback, I had also wanted to
> make the individual buckets be allocated as needed. That way if something
> was only doing allocations in, say, the 16 to 128 byte range, we wouldn't
> lose memory to track the (unused) higher order bucket sizes.
>
> I expect to send out the next revision after the coming merge window.

Thanks for the update.

-- 
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Micah Morton]

On Sat, Jan 4, 2025 at 7:49 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Dec 18, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
> > >
> > > syzbot attempts to write a buffer with a large size to a sysfs entry
> > > with writes handled by handle_policy_update(), triggering a warning
> > > in kmalloc.
> > >
> > > Check the size specified for write buffers before allocating.
> > >
> > > Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> > > Signed-off-by: Leo Stone <leocstone@gmail.com>
> > > ---
> > > v2: Make the check in handle_policy_update() to also cover
> > > safesetid_uid_file_write(). Thanks for your feedback.
> > > v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> > > ---
> > >  security/safesetid/securityfs.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > Looks okay to me.  Micah, are you planning to merge this patch, or
> > would you like me to take it via the LSM tree?
> >
> > Reviewed-by: Paul Moore <paul@paul-moore.com>
> >
> > I'm going to tag this to come back to it in a week or so in case we
> > don't hear from Micah, but if you don't see any further replies Leo,
> > feel free to send a gentle nudge ;)
>
> I'm going to go ahead and merge this into lsm/dev, if Micah has a
> problem with that I can always remove/revert if needed.
>
> We may need to revisit safesetid's documented support status, but
> that's a topic for another day.

Sorry guys, I missed this one. I usually just check in weekly on my
linux-security-users filter in my email inbox and see if any
discussions include 'SafeSetID'.

I must have skimmed over 'lsm: check size of writes' thinking it was a
generic LSM patch rather than something specific to SafeSetID.

I'll have to adjust my email settings so that emails which directly
list me on the CC are sent to a specific folder for me to check more
closely.

Micah

>
> Thanks everyone.
>
> --
> paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Paul Moore]

On Fri, Jan 24, 2025 at 2:24 PM Micah Morton <mortonm@chromium.org> wrote:
> On Sat, Jan 4, 2025 at 7:49 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Dec 18, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
> > > >
> > > > syzbot attempts to write a buffer with a large size to a sysfs entry
> > > > with writes handled by handle_policy_update(), triggering a warning
> > > > in kmalloc.
> > > >
> > > > Check the size specified for write buffers before allocating.
> > > >
> > > > Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> > > > Signed-off-by: Leo Stone <leocstone@gmail.com>
> > > > ---
> > > > v2: Make the check in handle_policy_update() to also cover
> > > > safesetid_uid_file_write(). Thanks for your feedback.
> > > > v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> > > > ---
> > > >  security/safesetid/securityfs.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > >
> > > Looks okay to me.  Micah, are you planning to merge this patch, or
> > > would you like me to take it via the LSM tree?
> > >
> > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> > >
> > > I'm going to tag this to come back to it in a week or so in case we
> > > don't hear from Micah, but if you don't see any further replies Leo,
> > > feel free to send a gentle nudge ;)
> >
> > I'm going to go ahead and merge this into lsm/dev, if Micah has a
> > problem with that I can always remove/revert if needed.
> >
> > We may need to revisit safesetid's documented support status, but
> > that's a topic for another day.
>
> Sorry guys, I missed this one. I usually just check in weekly on my
> linux-security-users filter in my email inbox and see if any
> discussions include 'SafeSetID'.
>
> I must have skimmed over 'lsm: check size of writes' thinking it was a
> generic LSM patch rather than something specific to SafeSetID.
>
> I'll have to adjust my email settings so that emails which directly
> list me on the CC are sent to a specific folder for me to check more
> closely.

No worries, I'm just happy to hear you're still around and looking out
for patches :)

I'm assuming you are okay with that patch?  If not we still have time
to fix it up.

-- 
paul-moore.com

Re: [PATCH v2] lsm: check size of writes

[Micah Morton]

On Fri, Jan 24, 2025 at 11:42 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jan 24, 2025 at 2:24 PM Micah Morton <mortonm@chromium.org> wrote:
> > On Sat, Jan 4, 2025 at 7:49 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Wed, Dec 18, 2024 at 4:51 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Dec 17, 2024 at 1:27 PM Leo Stone <leocstone@gmail.com> wrote:
> > > > >
> > > > > syzbot attempts to write a buffer with a large size to a sysfs entry
> > > > > with writes handled by handle_policy_update(), triggering a warning
> > > > > in kmalloc.
> > > > >
> > > > > Check the size specified for write buffers before allocating.
> > > > >
> > > > > Reported-by: syzbot+4eb7a741b3216020043a@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=4eb7a741b3216020043a
> > > > > Signed-off-by: Leo Stone <leocstone@gmail.com>
> > > > > ---
> > > > > v2: Make the check in handle_policy_update() to also cover
> > > > > safesetid_uid_file_write(). Thanks for your feedback.
> > > > > v1: https://lore.kernel.org/all/20241216030213.246804-2-leocstone@gmail.com/
> > > > > ---
> > > > >  security/safesetid/securityfs.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > >
> > > > Looks okay to me.  Micah, are you planning to merge this patch, or
> > > > would you like me to take it via the LSM tree?
> > > >
> > > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> > > >
> > > > I'm going to tag this to come back to it in a week or so in case we
> > > > don't hear from Micah, but if you don't see any further replies Leo,
> > > > feel free to send a gentle nudge ;)
> > >
> > > I'm going to go ahead and merge this into lsm/dev, if Micah has a
> > > problem with that I can always remove/revert if needed.
> > >
> > > We may need to revisit safesetid's documented support status, but
> > > that's a topic for another day.
> >
> > Sorry guys, I missed this one. I usually just check in weekly on my
> > linux-security-users filter in my email inbox and see if any
> > discussions include 'SafeSetID'.
> >
> > I must have skimmed over 'lsm: check size of writes' thinking it was a
> > generic LSM patch rather than something specific to SafeSetID.
> >
> > I'll have to adjust my email settings so that emails which directly
> > list me on the CC are sent to a specific folder for me to check more
> > closely.
>
> No worries, I'm just happy to hear you're still around and looking out
> for patches :)
>
> I'm assuming you are okay with that patch?  If not we still have time
> to fix it up.

Yes, looks good to me!

>
> --
> paul-moore.com