From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 20341 invoked from network); 3 Sep 2022 14:29:16 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 3 Sep 2022 14:29:16 -0000 Received: (qmail 16087 invoked by uid 550); 3 Sep 2022 14:29:12 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 16049 invoked from network); 3 Sep 2022 14:29:11 -0000 MIME-Version: 1.0 Date: Sat, 03 Sep 2022 17:28:56 +0300 From: Alexey Izbyshev To: musl@lists.openwall.com In-Reply-To: <20220903044820.GA8625@brightrain.aerifal.cx> References: <20220903001940.185345-1-izbyshev@ispras.ru> <20220903044820.GA8625@brightrain.aerifal.cx> User-Agent: Roundcube Webmail/1.4.4 Message-ID: <9fafafa1a8810b1d30798d499ebbb52f@ispras.ru> X-Sender: izbyshev@ispras.ru Content-Type: multipart/mixed; boundary="=_eac4002246e39fad649375a07c69b320" Subject: Re: [musl] [PATCH] fix potential ref count overflow in sem_open() --=_eac4002246e39fad649375a07c69b320 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; format=flowed On 2022-09-03 07:48, Rich Felker wrote: > 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. Indeed, my patch tried to fix the issue while staying within the wider logic of the current implementation. Since it makes the newly created semaphore visible via link() *before* re-locking semtab, the following scenario could happen: * thread 1 calls sem_open("/sem", O_CREAT) and proceeds until after link() succeeds * other threads successfully call sem_open("/sem", 0) INT_MAX times * thread 1 re-locks semtab, discovers another slot with the i_no it created, but can't increment the refcnt So in this case even though thread 1 did create a new semaphore, it still has to use an existing slot and hence can't back out (if it does, it'd look like semaphore creation didn't succeed, but opening the semaphore did). If link() and the refcnt increment occur under the same lock, this issue can be avoided, as in the attached draft patch. There is still a separate spurious failure case when after opening SEM_NSEMS_MAX semaphores an application can't open even an already opened one, but I don't think it's possible to completely fix in case O_CREAT is specified because we can't know whether we need to allocate a new slot or not before link() returns. Thanks, Alexey --=_eac4002246e39fad649375a07c69b320 Content-Transfer-Encoding: base64 Content-Type: text/x-diff; name=sem-open-refcnt.patch Content-Disposition: attachment; filename=sem-open-refcnt.patch; size=2372 ZGlmZiAtLWdpdCBhL3NyYy90aHJlYWQvc2VtX29wZW4uYyBiL3NyYy90aHJlYWQvc2VtX29wZW4u YwppbmRleCAwYWQyOWRlOS4uN2Y2NDhjMTkgMTAwNjQ0Ci0tLSBhL3NyYy90aHJlYWQvc2VtX29w ZW4uYworKysgYi9zcmMvdGhyZWFkL3NlbV9vcGVuLmMKQEAgLTM0LDcgKzM0LDcgQEAgc2VtX3Qg KnNlbV9vcGVuKGNvbnN0IGNoYXIgKm5hbWUsIGludCBmbGFncywgLi4uKQogCXZhX2xpc3QgYXA7 CiAJbW9kZV90IG1vZGU7CiAJdW5zaWduZWQgdmFsdWU7Ci0JaW50IGZkLCBpLCBlLCBzbG90LCBm aXJzdD0xLCBjbnQsIGNzOworCWludCBmZCwgaSwgZSwgc2xvdCwgZmlyc3Q9MSwgY3MsIGNyZWF0 ZWQ9MDsKIAlzZW1fdCBuZXdzZW07CiAJdm9pZCAqbWFwOwogCWNoYXIgdG1wWzY0XTsKQEAgLTU1 LDEzICs1NSw4IEBAIHNlbV90ICpzZW1fb3Blbihjb25zdCBjaGFyICpuYW1lLCBpbnQgZmxhZ3Ms IC4uLikKIAkvKiBSZXNlcnZlIGEgc2xvdCBpbiBjYXNlIHRoaXMgc2VtYXBob3JlIGlzIG5vdCBt YXBwZWQgeWV0OwogCSAqIHRoaXMgaXMgbmVjZXNzYXJ5IGJlY2F1c2UgdGhlcmUgaXMgbm8gd2F5 IHRvIGhhbmRsZQogCSAqIGZhaWx1cmVzIGFmdGVyIGNyZWF0aW9uIG9mIHRoZSBmaWxlLiAqLwot CXNsb3QgPSAtMTsKLQlmb3IgKGNudD1pPTA7IGk8U0VNX05TRU1TX01BWDsgaSsrKSB7Ci0JCWNu dCArPSBzZW10YWJbaV0ucmVmY250OwotCQlpZiAoIXNlbXRhYltpXS5zZW0gJiYgc2xvdCA8IDAp IHNsb3QgPSBpOwotCX0KLQkvKiBBdm9pZCBwb3NzaWJpbGl0eSBvZiBvdmVyZmxvdyBsYXRlciAq LwotCWlmIChjbnQgPT0gSU5UX01BWCB8fCBzbG90IDwgMCkgeworCWZvciAoc2xvdD0wOyBzbG90 PFNFTV9OU0VNU19NQVggJiYgc2VtdGFiW3Nsb3RdLnNlbTsgc2xvdCsrKTsKKwlpZiAoc2xvdCA9 PSBTRU1fTlNFTVNfTUFYKSB7CiAJCWVycm5vID0gRU1GSUxFOwogCQlVTkxPQ0sobG9jayk7CiAJ CXJldHVybiBTRU1fRkFJTEVEOwpAQCAtOTMsNiArODgsNyBAQCBzZW1fdCAqc2VtX29wZW4oY29u c3QgY2hhciAqbmFtZSwgaW50IGZsYWdzLCAuLi4pCiAJCQkJCWdvdG8gZmFpbDsKIAkJCQl9CiAJ CQkJY2xvc2UoZmQpOworCQkJCUxPQ0sobG9jayk7CiAJCQkJYnJlYWs7CiAJCQl9CiAJCQlpZiAo ZXJybm8gIT0gRU5PRU5UKQpAQCAtMTI4LDkgKzEyNCwxNiBAQCBzZW1fdCAqc2VtX29wZW4oY29u c3QgY2hhciAqbmFtZSwgaW50IGZsYWdzLCAuLi4pCiAJCQlnb3RvIGZhaWw7CiAJCX0KIAkJY2xv c2UoZmQpOworCisJCUxPQ0sobG9jayk7CiAJCWUgPSBsaW5rKHRtcCwgbmFtZSkgPyBlcnJubyA6 IDA7CiAJCXVubGluayh0bXApOwotCQlpZiAoIWUpIGJyZWFrOworCQlpZiAoIWUpIHsKKwkJCWNy ZWF0ZWQgPSAxOworCQkJYnJlYWs7CisJCX0KKwkJVU5MT0NLKGxvY2spOworCiAJCW11bm1hcCht YXAsIHNpemVvZihzZW1fdCkpOwogCQkvKiBGYWlsdXJlIGlzIG9ubHkgZmF0YWwgd2hlbiBkb2lu ZyBhbiBleGNsdXNpdmUgb3BlbjsKIAkJICogb3RoZXJ3aXNlLCBuZXh0IGl0ZXJhdGlvbiB3aWxs IHRyeSB0byBvcGVuIHRoZQpAQCAtMTQyLDEzICsxNDUsMTkgQEAgc2VtX3QgKnNlbV9vcGVuKGNv bnN0IGNoYXIgKm5hbWUsIGludCBmbGFncywgLi4uKQogCS8qIFNlZSBpZiB0aGUgbmV3bHkgbWFw cGVkIHNlbWFwaG9yZSBpcyBhbHJlYWR5IG1hcHBlZC4gSWYKIAkgKiBzbywgdW5tYXAgdGhlIG5l dyBtYXBwaW5nIGFuZCB1c2UgdGhlIGV4aXN0aW5nIG9uZS4gT3RoZXJ3aXNlLAogCSAqIGFkZCBp dCB0byB0aGUgdGFibGUgb2YgbWFwcGVkIHNlbWFwaG9yZXMuICovCi0JTE9DSyhsb2NrKTsKLQlm b3IgKGk9MDsgaTxTRU1fTlNFTVNfTUFYICYmIHNlbXRhYltpXS5pbm8gIT0gc3Quc3RfaW5vOyBp KyspOwotCWlmIChpPFNFTV9OU0VNU19NQVgpIHsKLQkJbXVubWFwKG1hcCwgc2l6ZW9mKHNlbV90 KSk7Ci0JCXNlbXRhYltzbG90XS5zZW0gPSAwOwotCQlzbG90ID0gaTsKLQkJbWFwID0gc2VtdGFi W2ldLnNlbTsKKwlpZiAoIWNyZWF0ZWQpIHsKKwkJZm9yIChpPTA7IGk8U0VNX05TRU1TX01BWCAm JiBzZW10YWJbaV0uaW5vICE9IHN0LnN0X2lubzsgaSsrKTsKKwkJaWYgKGk8U0VNX05TRU1TX01B WCkgeworCQkJbXVubWFwKG1hcCwgc2l6ZW9mKHNlbV90KSk7CisJCQlpZiAoc2VtdGFiW2ldLnJl ZmNudCA9PSBJTlRfTUFYKSB7CisJCQkJVU5MT0NLKGxvY2spOworCQkJCWVycm5vID0gRU1GSUxF OworCQkJCWdvdG8gZmFpbDsKKwkJCX0KKwkJCXNlbXRhYltzbG90XS5zZW0gPSAwOworCQkJc2xv dCA9IGk7CisJCQltYXAgPSBzZW10YWJbaV0uc2VtOworCQl9CiAJfQogCXNlbXRhYltzbG90XS5y ZWZjbnQrKzsKIAlzZW10YWJbc2xvdF0uc2VtID0gbWFwOwo= --=_eac4002246e39fad649375a07c69b320--