Re: [PATCH v7 04/11] LSM: syscalls for current process attributes

[Paul Moore]

On March 29, 2023 9:12:19 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:

...

> > +/**
> > + * security_setselfattr - Set an LSM attribute on the current process.
> > + * @attr: which attribute to set
> > + * @ctx: the user-space source for the information
> > + * @size: the size of the data
> > + * @flags: reserved for future use, must be 0
> > + *
> > + * Set an LSM attribute for the current process. The LSM, attribute
> > + * and new value are included in @ctx.
> > + *
> > + * Returns 0 on success, an LSM specific value on failure.
> > + */
> > +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> > +                        size_t __user size, u32 __user flags)
> > +{
> > +       struct security_hook_list *hp;
> > +       struct lsm_ctx lctx;
>
> Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
> > +       if (flags != 0)
> > +               return -EINVAL;
> > +       if (size < sizeof(*ctx))
> > +               return -EINVAL;
>
> If we're only going to support on 'lsm_ctx' entry in this function we
> should verify that the 'len' and 'ctx_len' fields are sane.  Although
> more on this below ...
>
> > +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> > +               return -EFAULT;
> > +
> > +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> > +               if ((hp->lsmid->id) == lctx.id)
> > +                       return hp->hook.setselfattr(attr, ctx, size, flags);
>
> Can anyone think of any good reason why we shouldn't support setting
> multiple LSMs in one call, similar to what we do with
> security_getselfattr()?  It seems like it might be a nice thing to
> have ...

Scratch that, I just reminded myself why attempting to set an attribute on 
multiple LSMs in one operation would be a nightmare. Sorry about the confusion.

--
paul-moore.com