On Wed, Aug 02, 2023 at 10:44:27AM -0700, Casey Schaufler wrote:
> +/**
> + * security_setselfattr - Set an LSM attribute on the current process.
> + * @attr: which attribute to set
> + * @ctx: the user-space source for the information
Would be more idiomatic to name the user arg uctx.
> + * @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, -EINVAL if the input is inconsistent, -EFAULT
> + * if the user buffer is inaccessible or an LSM specific failure.
> + */
> +int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
> + size_t size, u32 flags)
> +{
> + struct security_hook_list *hp;
> + struct lsm_ctx *lctx;
> + int rc = LSM_RET_DEFAULT(setselfattr);
> +
> + if (flags)
> + return -EINVAL;
> + if (size < sizeof(*ctx))
> + return -EINVAL;
> +
I think it would be cleaner to check against lctx.
But the actual point is that you want an upper bound here.
> + lctx = kmalloc(size, GFP_KERNEL);
> + if (lctx == NULL)
> + return -ENOMEM;
> +
> + if (copy_from_user(&lctx, ctx, size))
> + return -EFAULT;
> +
One commenter over already mentioned this should be lctx, not &lctx.
> + if (size < lctx->len || size < lctx->ctx_len + sizeof(ctx) ||
> + lctx->len < lctx->ctx_len + sizeof(ctx))
> + return -EINVAL;
> +
This leaks lctx.
> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> + if ((hp->lsmid->id) == lctx->id) {
> + rc = hp->hook.setselfattr(attr, lctx, size, flags);
> + break;
> + }
> +
> + kfree(lctx);
> + return rc;
> +}
> +
