Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()

From: Paul Moore <paul@paul-moore.com>
To: "Mickaël Salaün" <mic@digikod.net>
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	Casey Schaufler <casey@schaufler-ca.com>,
	John Johansen <john.johansen@canonical.com>
Subject: Re: [RFC PATCH 3/3] lsm: consolidate buffer size handling into lsm_fill_user_ctx()
Date: Thu, 26 Oct 2023 11:38:20 -0400	[thread overview]
Message-ID: <CAHC9VhTFfBVb3Ru3fhNgRV=c82UhHmhcvcpYDPUFW9G8LNHjHA@mail.gmail.com> (raw)
In-Reply-To: <20231026.ieyieDie4see@digikod.net>

On Thu, Oct 26, 2023 at 11:13 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Tue, Oct 24, 2023 at 05:35:29PM -0400, Paul Moore wrote:
> > While we have a lsm_fill_user_ctx() helper function designed to make
> > life easier for LSMs which return lsm_ctx structs to userspace, we
> > didn't include all of the buffer length safety checks and buffer
> > padding adjustments in the helper.  This led to code duplication
> > across the different LSMs and the possibility for mistakes across the
> > different LSM subsystems.  In order to reduce code duplication and
> > decrease the chances of silly mistakes, we're consolidating all of
> > this code into the lsm_fill_user_ctx() helper.
> >
> > The buffer padding is also modified from a fixed 8-byte alignment to
> > an alignment that matches the word length of the machine
> > (BITS_PER_LONG / 8).
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
>
> > diff --git a/security/security.c b/security/security.c
> > index 67ded406a5ea..45c4f5440c95 100644
> > --- a/security/security.c
> > +++ b/security/security.c

...

> > +int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> > +                   void *val, size_t val_len,
> > +                   u64 id, u64 flags)
> >  {
> > -     struct lsm_ctx *lctx;
> > -     size_t locallen = struct_size(lctx, ctx, context_size);
> > +     struct lsm_ctx *nctx = NULL;
> > +     size_t nctx_len;
> >       int rc = 0;
> >
> > -     lctx = kzalloc(locallen, GFP_KERNEL);
> > -     if (lctx == NULL)
> > -             return -ENOMEM;
> > +     nctx_len = ALIGN(struct_size(nctx, ctx, val_len), BITS_PER_LONG / 8);
>
> Why the arch-dependent constant?

My thinking is that most arches tend to perform better when data is
aligned on a word boundary and this should help achieve that in a way
that doesn't assume the arch's word length.  If you have an idea on
how to do this differently I'm open to suggestions.

It's worth noting that this is something we can change in the future
as the lsm_ctx struct has the len field which we can use for arbitrary
amounts of padding, including none.

> I'm not even sure why we want to align this size. We'll only copy the
> actual size right?

We allocate, zero out, and copy @nctx_len/@nctx->len.

> > +     if (nctx_len > *uctx_len) {
> > +             rc = -E2BIG;
> > +             goto out;
> > +     }
> >
> > -     lctx->id = id;
> > -     lctx->flags = flags;
> > -     lctx->ctx_len = context_size;
> > -     lctx->len = locallen;
> > +     nctx = kzalloc(nctx_len, GFP_KERNEL);
> > +     if (nctx == NULL) {
> > +             rc = -ENOMEM;
> > +             goto out;
> > +     }
> > +     nctx->id = id;
> > +     nctx->flags = flags;
> > +     nctx->len = nctx_len;
> > +     nctx->ctx_len = val_len;
> > +     memcpy(nctx->ctx, val, val_len);
> >
> > -     memcpy(lctx->ctx, context, context_size);
> > -
> > -     if (copy_to_user(ctx, lctx, locallen))
> > +     if (copy_to_user(uctx, nctx, nctx_len))
> >               rc = -EFAULT;
> >
> > -     kfree(lctx);
> > -
> > +out:
> > +     kfree(nctx);
> > +     *uctx_len = nctx_len;
> >       return rc;
> >  }

-- 
paul-moore.com