Re: [PATCH] ipc/sem: check nsops overflow in do_semtimedop()

[Lorenzo Stoakes]

On Mon, Jun 29, 2026 at 02:57:05PM +0800, Yi Xie wrote:
> do_semtimedop() caps nsops against sc_semopm, but still multiplies it by
> sizeof(struct sembuf) when copying from userspace.  That product can
> overflow, so copy_from_user() may not match what kvmalloc_objs() allocated.

This is just incorrect, becasue for copy_from_user() to happen kvmalloc_objs()
would be called -> __alloc_objs() -> size_mul() (saturates) -> -ENOMEM.

>
> Use check_mul_overflow() and return -E2BIG on overflow.
>
> Signed-off-by: Yi Xie <xieyi@xxxxxxxxxx>
> ---
>  ipc/sem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 5ec41de7e85b..791a0505923b 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -86,6 +86,7 @@
>  #include <linux/ipc_namespace.h>
>  #include <linux/sched/wake_q.h>
>  #include <linux/nospec.h>
> +#include <linux/overflow.h>
>  #include <linux/rhashtable.h>
>
>  #include <linux/uaccess.h>
> @@ -2225,6 +2226,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
>  	struct sembuf fast_sops[SEMOPM_FAST];
>  	struct sembuf *sops = fast_sops;
>  	struct ipc_namespace *ns;
> +	size_t sops_bytes;

Maybe just len or size?

>  	int ret;
>
>  	ns = current->nsproxy->ipc_ns;
> @@ -2232,6 +2234,8 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
>  		return -E2BIG;
>  	if (nsops < 1)
>  		return -EINVAL;
> +	if (check_mul_overflow(nsops, sizeof(*sops), &sops_bytes))
> +		return -E2BIG;

struct sembuf is 6 bytes is size, so you'd need some truly enormous
ns->sc_semopm (defaults to SEMOPM == 500) and nsops to get an overflow here, and
it'd only be the case on 32-bit systems also.

But in any case as per above,

kvmalloc_objs() -> __alloc_objs() -> size_mul() (saturates) -> -ENOMEM.

So this check is redundant.

>
>  	if (nsops > SEMOPM_FAST) {
>  		sops = kvmalloc_objs(*sops, nsops);
> @@ -2239,7 +2243,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
>  			return -ENOMEM;
>  	}
>
> -	if (copy_from_user(sops, tsops, nsops * sizeof(*tsops))) {
> +	if (copy_from_user(sops, tsops, sops_bytes)) {
>  		ret =  -EFAULT;
>  		goto out_free;
>  	}
> --
> 2.25.1
>

Thanks, Lorenzo