Re: [musl] [PATCH] fix potential ref count overflow in sem_open()

[Rich Felker <dalias@libc.org>]

On Sat, Sep 03, 2022 at 03:19:40AM +0300, Alexey Izbyshev wrote:
> sem_open() attempts to avoid overflow on the future ref count increment
> by computing the sum of all ref counts in semtab and checking that it's
> not INT_MAX when semtab is locked for slot reservation.  This approach
> suffers from a TOCTTOU: by the time semtab is re-locked after opening a
> semaphore, the sum could have already been increased by a concurrent
> sem_open(), so it will overflow on the increment. An individual ref
> count can be overflowed as well if the call happened to open a duplicate
> of the only other semaphore.
> 
> Moreover, since the overflow avoidance check admits a negative (i.e.
> overflowed) sum, after this state is reached, an individual ref count
> can be incremented until it reaches 1 again, and then sem_close() will
> incorrectly free the semaphore, promoting what could be just a
> signed-overflow UB to a use-after-free.
> 
> Fix the overflow check by accounting for the maximum possible number of
> concurrent sem_open() calls that have already reserved a slot, but
> haven't incremented the ref count yet.
> ---
> Alternatively, we could use the number of currently reserved slots
> instead of SEM_NSEMS_MAX, preserving the ability of individual ref
> counts to reach INT_MAX in non-concurrent scenarios. I'm not sure
> whether it matters, so I'm sending a smaller of the two fixes.
> 
> Alexey
> ---
>  src/thread/sem_open.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/thread/sem_open.c b/src/thread/sem_open.c
> index 0ad29de9..611a3f64 100644
> --- a/src/thread/sem_open.c
> +++ b/src/thread/sem_open.c
> @@ -61,7 +61,7 @@ sem_t *sem_open(const char *name, int flags, ...)
>  		if (!semtab[i].sem && slot < 0) slot = i;
>  	}
>  	/* Avoid possibility of overflow later */
> -	if (cnt == INT_MAX || slot < 0) {
> +	if (cnt > INT_MAX - SEM_NSEMS_MAX || slot < 0) {
>  		errno = EMFILE;
>  		UNLOCK(lock);
>  		return SEM_FAILED;
> -- 
> 2.37.2

Thanks! This is probably acceptable at least relative to the current
behavior, but thinking about it, the current behavior (this whole
logic) doesn't really make sense. If flags are such that a new
semaphore can't be created, the claim that there's no way to handle
failure after opening is false; the operation is side-effect free and
we can just back out. The only case where we can't back out is when
we're creating a new semaphore, and in that case, it will never be
incrementing the refcnt on an existing slot; it will always be a new
slot. And even in this case, I think we could back out if we needed
to, since the file is created initially with a temp name.

My leaning is towards accepting your patch as-is as a bug fix, then
trying to fix this not to have gratuitous failure cases where an
excessive open count on one semaphore wrongly precludes opening
others.

Rich