[PATCH v2] lsm: check size of writes

Leo Stone posted 1 patch 12 months ago
security/safesetid/securityfs.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH v2] lsm: check size of writes
Posted by Leo Stone 12 months ago
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
Posted by Paul Moore 12 months ago
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
Posted by Paul Moore 11 months, 2 weeks ago
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
Posted by Micah Morton 10 months, 3 weeks ago
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
Posted by Paul Moore 10 months, 3 weeks ago
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
Posted by Micah Morton 10 months, 3 weeks ago
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
Re: [PATCH v2] lsm: check size of writes
Posted by Tetsuo Handa 12 months ago
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
Posted by Paul Moore 11 months, 2 weeks ago
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
Posted by Tetsuo Handa 12 months ago
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
Posted by Kees Cook 11 months, 4 weeks ago
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
Posted by Paul Moore 11 months, 2 weeks ago
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
Posted by Kees Cook 11 months, 2 weeks ago
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
Posted by Paul Moore 11 months, 1 week ago
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