| Message ID | 20241217182657.10080-2-leocstone@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] lsm: check size of writes | expand |
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
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.
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?
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;
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(+)