From patchwork Wed May 7 16:08:40 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: dbgbgtf X-Patchwork-Id: 111687 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D59B1385801B for ; Wed, 7 May 2025 16:10:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D59B1385801B Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=irtsT3e9 X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id 7AAD53857BBA for ; Wed, 7 May 2025 16:09:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7AAD53857BBA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7AAD53857BBA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42e ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1746634156; cv=none; b=KZi+pCu7FQ1oDrVM6IrQeDYqqDyzhswqyj4SAvbM+xdmU32uAkQMV+45Xh2zsq0dwqInjj5BHoXoMNhk+Ue/N3gEQTtLpScnHY6ookMKqvz+0qvWoDcOe56kW8nmVQirjxFSymzTdTZYJIltCWaPIOysBWcNTCHDiJdpxoOPJPk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1746634156; c=relaxed/simple; bh=60d2sUgoQHqP2asGvyw4h+D9VM0BJ7RsAFYR5p0jWSk=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=eFKdUBpxiHiPsANGnILhRQ86vvdMsWEpaNJfMAYkxtlqbEurkKUWBMwZBwiSFMLcyg0BXMhz96G6aWcerptbu2tPRmnIgcFVgL3xuswnZ7Fpm34dYvpCdcfNXB4ATY6+fD2tXfWKBvpC4l7jnXrKddWKua3Uyf1CStFDWKFTHmw= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7AAD53857BBA Received: by mail-pf1-x42e.google.com with SMTP id d2e1a72fcca58-7399838db7fso116624b3a.0 for ; Wed, 07 May 2025 09:09:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1746634155; x=1747238955; darn=sourceware.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=5xXzjcCRBRrVGtJwfcTSaC7Jg67STiTlv2EygboWvEU=; b=irtsT3e97Y16AfaeWCWpypTsVfXxA6S0ZPvjU1WzpOhz/63CLJN9CuLVwJYaYHmz0E STHsHCquI2YsKYTTl7bJeOC+AzwiFwl1VBboiOPgxRTOrJUx9RC2e6P+uEJRNqAXc7Hn wZLmKCGBmFiRhNShY5rupE9AX8NGZDVkHDUISA8CvfSvsyu3hn9gGDMtl42Z4Kw4Ns4G bR7Pg6oC6SQOkBGffWRbKdpJ3W3oqL7PWPSXWLZavh29tT1pzptpzoqAuMyHzd6f+cek JXudx9GtBdeu1hxG6oizHhYRJEcJNOkfa/MigAwM2GQMxAP3Pe1xlXynxlZuRO6Nku6b tJ+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1746634155; x=1747238955; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5xXzjcCRBRrVGtJwfcTSaC7Jg67STiTlv2EygboWvEU=; b=Kql/hkLnozE1QsGDSxF8nYY84jj4DiTPFfTbonZCvKqibNX2aCsreAJSI3GZ39DgUA t9/fBDx9Cb1sWz5Y93JH3Sy7FPjv/q4XsGtKC3ICaCEjnHKO7R/wsblpbkuGJIiv+2Sm /OEkVIQtcto6Edd6VEIeQkl1r3BQPos34HTygYOiBo7iRcFmaYriFZ+uWIsLeBdefU/d 4kdb3NHNXGEh1xIwGl+SPB5qj0rOLs+FQ0mQYIDsRdFZqoZhhIu4a01sUiG5NLQdcjtU 8pG4OQl0ProqJBqTzwGQ8a1TYe0vU+YFYJWqY7BmHkVSi+3WnjPBesk10wuSQDdVelgy qMVg== X-Forwarded-Encrypted: i=1; AJvYcCW6692z5YSG5AsuPVACcgM5mMZmzAMiGk3kNizpn1FbV682WNNjelXTu9nWQaOZRjv9IF9ztxpYb3wk@sourceware.org X-Gm-Message-State: AOJu0YwbyzAWvpUvG51DQJnM1n/cGNE145nbLvkRwrUM820xiMNraMpW 7NbhJAqtaq0QizwyYweKj56LDuw18uvY2pLyq+EVyaWl/FP7dzpg X-Gm-Gg: ASbGnctBa8lVZajjAT6fysn2/18+SVp2EiYE5vGsnTa4jvMq0tyuCCxT2oRJPTxMzrQ I5XVktHht14rSHJThrk1wd/xQHCHe5s6HphRrG7yP276r5/XKa0i9Alyoz1Dy0V0d5Lx3xE09dP d2y0XreH5QrjNtw8EzGtP1FSHFusI9q+f1l6nqZ7hsbHZgf6eOKDhWmvEEHzcOOO/C8NKq8QUrq j6TcOOZtKfC7iPNU4dTewWa3GePvO3r/YV1DlxXhOZ+kDjYrX4a1FEd8nXs3TsO3Zt3PVQIxxiA iAK7cYhcFMimdtzBUnd4pl4brGjQG/b2srU72UJAvBYiUAR/3UVBONzSOKF2ZFRMy9FWvTyQoUA A X-Google-Smtp-Source: AGHT+IEi4PXz67xiRGVNFDm68n0/iI+k4riCZ1CxzydeyU+LQf1spocsLJlgYqN9V5ENdO7csLStBw== X-Received: by 2002:a05:6a00:1b4c:b0:732:706c:c4ff with SMTP id d2e1a72fcca58-740a94e60d7mr68999b3a.7.1746634155329; Wed, 07 May 2025 09:09:15 -0700 (PDT) Received: from localhost (114-32-91-205.hinet-ip.hinet.net. [114.32.91.205]) by smtp.gmail.com with UTF8SMTPSA id d2e1a72fcca58-74058dbc12fsm11706675b3a.64.2025.05.07.09.09.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 May 2025 09:09:14 -0700 (PDT) From: dbgbgtf X-Google-Original-From: dbgbgtf To: wilco.dijkstra@arm.com Cc: dudududumaxver@gmail.com, libc-alpha@sourceware.org Subject: [PATCH] malloc: check tcache mem size in tcache_get_n to avoid arbitrary mem allocation Date: Thu, 8 May 2025 00:08:40 +0800 Message-ID: <20250507160848.1815570-1-dudududuMaxVer@gmail.com> X-Mailer: git-send-email 2.49.0 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HK_RANDOM_ENVFROM, HK_RANDOM_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patchwork=sourceware.org@sourceware.org Hi > The goal is to detect heap corruption cheaply and as early as possible. I understood, but checks in `tcache_put` is too early. And hackers can break the pointer swizzing after `tcache_put` easily. > however if we wanted to make malloc > safer, we'd have to stop using data structures that are stored in chunks. I agree. But that is not up to me, I just want to add a tiny check. > Tcache_put is used in several places on blocks that have been freed for a while > and were stored in different lists, so they could have been corrupted at that point. I don't want to rely on such a possibility, it's not safe. Adding a check that can likely be bypassed is worse than having none at all. Checks in `tcache_get` is effective and the cost is acceptable, while checks in tcache_put costs and failed to provide meaningful validation. > tcache_get already does a check - what I'm saying is that we can improve it. Yeah, I mean why not put my check and align check in `tcache_put` together. > This works, is cheap and triggers in your testcase. If you look closely, your check can incorrectly terminate legitimate free operations. (the malloc diff shows how I implemented your suggested patch) In my new tst, your check leads to `malloc(): unaligned tcache chunk detected` before dangerous instructions. Anyway, I really don’t want to keep arguing about where my check should be placed. This simple patch has already taken up too much of my time. If your only concern about my patch is where the check should be placed now, I suggest merging my check into the mainline first. If you agree, I'll send the final version of my patch in the next email. And then you can open a separate commit to discuss whether my new check—or any other checks—should be relocated. I’ll continue to follow up and try to provide feedback. Thanks, dbgbgtf Signed-off-by: dbgbgtf --- malloc/malloc.c | 3 + malloc/tst-tcache-arbitrary-alloc.c | 90 +++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 malloc/tst-tcache-arbitrary-alloc.c diff --git a/malloc/malloc.c b/malloc/malloc.c index 9d860eac9c..050fc00cd7 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3163,6 +3163,9 @@ tcache_put (mchunkptr chunk, size_t tc_idx) { tcache_entry *e = (tcache_entry *) chunk2mem (chunk); + if (__glibc_unlikely (!aligned_OK (REVEAL_PTR (e->next)))) + malloc_printerr ("malloc(): unaligned tcache chunk detected"); + /* Mark this chunk as "in the tcache" so the test in __libc_free will detect a double free. */ e->key = tcache_key; diff --git a/malloc/tst-tcache-arbitrary-alloc.c b/malloc/tst-tcache-arbitrary-alloc.c new file mode 100644 index 0000000000..7941040d92 --- /dev/null +++ b/malloc/tst-tcache-arbitrary-alloc.c @@ -0,0 +1,90 @@ +#include +#include +#include +#include +#include +#include +#define INTERNAL_SIZE_T size_t + +#define mem2chunk(p) ((void *)((char *)(p) - CHUNK_HDR_SZ)) +#define SIZE_SZ (sizeof (INTERNAL_SIZE_T)) +#define CHUNK_HDR_SZ (2 * SIZE_SZ) + +struct malloc_chunk +{ + + INTERNAL_SIZE_T mchunk_prev_size; /* Size of previous chunk (if free). */ + INTERNAL_SIZE_T mchunk_size; /* Size in bytes, including overhead. */ + + struct malloc_chunk *fd; /* double links -- used only if free. */ + struct malloc_chunk *bk; + + /* Only used for large blocks: pointer to next larger size. */ + struct malloc_chunk *fd_nextsize; /* double links -- used only if free. */ + struct malloc_chunk *bk_nextsize; +}; + +typedef struct malloc_chunk *mchunkptr; + +void danger() +{ + void *ptr[4]; + ptr[0] = malloc (0x100); + ptr[1] = malloc (0x100); + // malloc aleast two chunk at the same size + + free (ptr[1]); + free (ptr[0]); + // free the second one, and the the first one + // the tcachebin will be tcache->ptr[0]->ptr[1] + + ptr[0] = mem2chunk (ptr[0]); + + // checking if ptr[2] address is correctly aligned like 0x7fff ffff fff0 + // if ptr[2] is not correctly aligned, use ptr[3] instead + // which should be a correctly aligned address + bool is_wrong_align = (INTERNAL_SIZE_T)&ptr[2] & 0x8; + if (is_wrong_align) + { + printf ("target address at: %p\n", &ptr[3]); + ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12) + ^ (INTERNAL_SIZE_T)(&ptr[3])); + } + else + { + printf ("target address at: %p\n", &ptr[2]); + ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12) + ^ (INTERNAL_SIZE_T)(&ptr[2])); + } + // if we change the ptr[0]->fd + // the bin will be tcache->ptr[0]->anywhere + // I use ptr[2] or ptr[3] address as example + + ptr[1] = malloc (0x100); + ptr[0] = malloc (0x100); + // and the malloc them, take the tcachebin out + // to see if we have target address + + printf ("ptr[0] point at: %p", ptr[0]); + // so we get target address now +} + +void safe() +{ + void *ptr[4]; + ptr[0] = malloc(0x100); + ptr[1] = malloc(0x100); + + free (ptr[1]); + free (ptr[0]); +} + +int +main () +{ + printf("here are some safe instructions\n"); + safe(); + + printf("here are some danger instructions\n"); + danger(); +}