From patchwork Wed Nov 8 13:28:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arjun Shankar X-Patchwork-Id: 835808 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-86891-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="V0iVEdLh"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3yX6Zh0h1Mz9sMN for ; Thu, 9 Nov 2017 00:28:59 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; q=dns; s=default; b=gi5Ls9JE6XnmlXjT twr9S3t8o2j9mBZr85uEDl/PKo+RLPj83s2Y+NgXs6tczUiCMVo4y37Czv1rLl3f qy6ageJ/S4E8kQ2tk1SymP7e9W1MuvS6pM7oG3AcjQhdoYqoeZ6vHrUKpOhLPZGC gdGxTySuB3cCnV86x1paG1ENDck= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; s=default; bh=izTNFK92M5HBxb+p1RnDGw DKw6w=; b=V0iVEdLhFf5fzONVwPS5yC51M0dfXtLPXHyifuvV7aKcwSnhNaLP9d WRrVsrgaYquHImugb1XZNAskP9gJAyph6mpFHPWpgg+GRuQ1KKqkM2L8TvipDgfC mrdRBxV17F1T82i57845ElhK1bTGE0vHZEbMKxyjT9/SbeyyDSyvk= Received: (qmail 4913 invoked by alias); 8 Nov 2017 13:28:51 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 4888 invoked by uid 89); 8 Nov 2017 13:28:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=1150 X-HELO: aloka.lostca.se Date: Wed, 8 Nov 2017 13:28:44 +0000 From: Arjun Shankar To: libc-alpha@sourceware.org Cc: DJ Delorie , Carlos O'Donell , Siddhesh Poyarekar Subject: [PATCH v2] Fix integer overflow in malloc when tcache is enabled [BZ #22375] Message-ID: <20171108132844.GA79361@aloka.lostca.se> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) When the per-thread cache is enabled, __libc_malloc uses request2size (which does not perform an overflow check) to calculate the chunk size from the requested allocation size. This leads to an integer overflow causing malloc to incorrectly return the last successfully allocated block when called with a very large size argument (close to SIZE_MAX). This patch uses checked_request2size instead, removing the overflow, and adds a new test to guard against regressions. ChangeLog: 2017-11-06 Arjun Shankar [BZ #22375] * malloc/malloc.c (__libc_malloc): Use checked_request2size instead of request2size. * malloc/tst-malloc-too-large.c: New test. * malloc/Makefile: Add tst-malloc-too-large. --- Florian has flagged the bug as 'security+' in bugzilla and will eventually get a CVE ID for it. v1 discussion: https://sourceware.org/ml/libc-alpha/2017-11/msg00192.html v2 includes a top level comment in the test source and includes a missing call to 'free' in it. malloc/Makefile | 1 + malloc/malloc.c | 3 +- malloc/tst-malloc-too-large.c | 150 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 malloc/tst-malloc-too-large.c diff --git a/malloc/Makefile b/malloc/Makefile index 7ae3d82..a01279d 100644 --- a/malloc/Makefile +++ b/malloc/Makefile @@ -35,6 +35,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ tst-interpose-thread \ tst-alloc_buffer \ tst-malloc-tcache-leak \ + tst-malloc-too-large \ tests-static := \ tst-interpose-static-nothread \ diff --git a/malloc/malloc.c b/malloc/malloc.c index f94d51c..2ebd97f 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3022,7 +3022,8 @@ __libc_malloc (size_t bytes) return (*hook)(bytes, RETURN_ADDRESS (0)); #if USE_TCACHE /* int_free also calls request2size, be careful to not pad twice. */ - size_t tbytes = request2size (bytes); + size_t tbytes; + checked_request2size (bytes, tbytes); size_t tc_idx = csize2tidx (tbytes); MAYBE_INIT_TCACHE (); diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c new file mode 100644 index 0000000..48d1088 --- /dev/null +++ b/malloc/tst-malloc-too-large.c @@ -0,0 +1,150 @@ +/* Test and verify that too-large memory allocations fail with ENOMEM. + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* Bug 22375 [1] reported a regression in malloc where if after malloc'ing + then free'ing a small block of memory, malloc is then called with a + really large size argument (close to SIZE_MAX): instead of returning NULL + and setting errno to ENOMEM, malloc incorrectly returns the previously + allocated block instead. This test guards against such regressions by + repeatedly allocating and freeing small blocks of memory then trying to + allocate various block sizes larger than the memory bus width of 64-bit + targets supported by glibc. The test verifies that such impossibly large + allocations correctly fail. + + [1] https://sourceware.org/bugzilla/show_bug.cgi?id=22375 */ + + +#include + + +#if __WORDSIZE < 64 + + +static int +do_test (void) +{ + FAIL_UNSUPPORTED ("Test does not support targets with word size < 64-bit"); +} + + +#else /* __WORDSIZE >= 64 */ + + +#include +#include +#include + + +/* This function prepares for each 'too-large memory allocation' test by + performing a small successful malloc/free and resetting errno prior to + the actual test. */ +static void +test_setup (void) +{ + void * ptr = malloc (16); + TEST_VERIFY_EXIT (ptr != NULL); + free (ptr); + errno = 0; +} + + +/* This function tests each of: + - malloc (SIZE) + - calloc (1, SIZE) + - calloc (SIZE, 1) + - realloc (PTR_FOR_REALLOC, SIZE) + and precedes each of these tests with a small malloc before it. */ +static void +test_large_allocations (size_t size) +{ + void * ptr_to_realloc; + test_setup (); + TEST_VERIFY (malloc (size) == NULL); + TEST_VERIFY (errno == ENOMEM); + + test_setup (); + TEST_VERIFY (calloc (size, 1) == NULL); + TEST_VERIFY (errno == ENOMEM); + + test_setup (); + TEST_VERIFY (calloc (1, size) == NULL); + TEST_VERIFY (errno == ENOMEM); + + ptr_to_realloc = malloc (16); + TEST_VERIFY_EXIT (ptr_to_realloc != NULL); + test_setup (); + TEST_VERIFY (realloc (ptr_to_realloc, size) == NULL); + TEST_VERIFY (errno == ENOMEM); + free (ptr_to_realloc); +} + + +#define FOURTEEN_ON_BITS ((1UL << 14) - 1) +#define FIFTY_ON_BITS ((1UL << 50) - 1) + + +static int +do_test (void) +{ + size_t lsbs, msbs, size; + + DIAG_PUSH_NEEDS_COMMENT; +#if __GNUC_PREREQ (7, 0) + /* GCC 7 warns about too-large allocations; here we want to test + that they fail. */ + DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than="); +#endif + + /* The allocation SIZE used during each iteration is obtained by combining + LSBS and MSBS. In order to catch all interesting cases but avoid + looping through every too-large size, i.e. [2^50, 2^64), we will have + two loops with different iteration patterns for LSBs and MSBs. */ + + /* Loop 1: Ensure that all allocations with SIZE close to SIZE_MAX fail: + The 14 MSBs are "all ON" during every iteration, + the 50 LSBs are decremented starting from "all ON", for 1<<14 + iterations. */ + for (lsbs = FIFTY_ON_BITS; lsbs > FIFTY_ON_BITS - (1UL << 14); lsbs--) + { + size = (FOURTEEN_ON_BITS << 50) | lsbs; + test_large_allocations (size); + } + + /* Loop 2: Test at intervals of (1 << 50) that allocation sizes ranging + from SIZE_MAX down to (1 << 50) fail: + The 14 MSBs are decremented starting from "all ON" going down to 1, + the 50 LSBs are "all ON" and then "all OFF" during every iteration. */ + for (msbs = FOURTEEN_ON_BITS; msbs >= 1; msbs--) + { + size = (msbs << 50) | FIFTY_ON_BITS; + test_large_allocations (size); + + size = msbs << 50; + test_large_allocations (size); + } + + DIAG_POP_NEEDS_COMMENT; + + return 0; +} + + +#endif /* __WORDSIZE >= 64 */ + + +#include