| Message ID | 20241216030213.246804-2-leocstone@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | lsm: check size of writes | expand |
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);
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);
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);
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(-)