Messages in this thread | | | Date | Fri, 25 Aug 2023 02:12:57 +0200 | From | Mateusz Guzik <> | Subject | Re: [PATCH v13 04/11] LSM: syscalls for current process attributes |
| |
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; > +} > +
| |