[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220903044820.GA8625@brightrain.aerifal.cx>
Date: Sat, 3 Sep 2022 00:48:20 -0400
From: Rich Felker <dalias@...c.org>
To: Alexey Izbyshev <izbyshev@...ras.ru>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] fix potential ref count overflow in sem_open()
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
