linux-kernel - Re: [PATCH v13 04/11] LSM: syscalls for current process attributes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230825001257.qcckxswcrehs4uh2@f>
Date:   Fri, 25 Aug 2023 02:12:57 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     paul@...l-moore.com, linux-security-module@...r.kernel.org,
        jmorris@...ei.org, serge@...lyn.com, keescook@...omium.org,
        john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
        stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, mic@...ikod.net
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;
> +}
> +