Message ID | 20231113225835.4083255-2-ppluzhnikov@google.com |
---|---|
State | Committed |
Headers |
Return-Path: <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> 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 42357392C293 for <patchwork@sourceware.org>; Mon, 13 Nov 2023 23:00:18 +0000 (GMT) X-Original-To: elfutils-devel@sourceware.org Delivered-To: elfutils-devel@sourceware.org Received: from mail-pg1-x549.google.com (mail-pg1-x549.google.com [IPv6:2607:f8b0:4864:20::549]) by sourceware.org (Postfix) with ESMTPS id 398C238560B9 for <elfutils-devel@sourceware.org>; Mon, 13 Nov 2023 23:00:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 398C238560B9 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=flex--ppluzhnikov.bounces.google.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 398C238560B9 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::549 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699916411; cv=none; b=pQS7S2SMoiF9a3IbsVdZQQ4kzgc8o9OqtbAJQpsv7Wgi7Tg+Wpwqr3SYUUVLVi/iZ/lRtofPT/+8i3EwhJPciInJzosW56BOHtOBS2AXfO6pcahMso2e8tBUxsIBRQbxj2vl0+7hd6lB61C1MScfz2TGYZQag23UzBukITyUzok= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699916411; c=relaxed/simple; bh=EMtay5NLksyMflkJSbcNHOuL/mU4LMyo87Wtkd48v/I=; h=DKIM-Signature:Date:Mime-Version:Message-ID:Subject:From:To; b=DqqMP6r4CwToX/SncPF3l0hNp4Fvh4tuEtUn7REGYh5daA/UHP6Ndc7TWddFKPZvrzmvPFWXLDLtJEVptONT7hqZdZNyrqzdaZLd91CVTJlIAf/yK2QtC0MtXH546XFsTMLbKYP8fbB3QvA+Dex9TebztynJ9fBMS86jkQRpaHQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pg1-x549.google.com with SMTP id 41be03b00d2f7-5b914c0b7c8so4534347a12.2 for <elfutils-devel@sourceware.org>; Mon, 13 Nov 2023 15:00:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1699916409; x=1700521209; darn=sourceware.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=E3N8WVoRLOkhjBrZv4IOmgMW6wphPHeM3tdZaQUJCDA=; b=AwYTNWJjVIUAvu3F7VGQMZw+C5hwcXJ7nNsIuMEs9iGsFD0JhIDQn5z8XKFIb3bFAH 8ncDcXTYUHGG8vDNA7nlKc2fDTTEjI2O+nCrNoE3obfyZtPtcZbug4jRS/5xWWxCUgim 9dO3RaL71IdSdb0ClidPk5HGfCfdDSvKocRs5MxnyHiyItSWjRGx9kxWxfXcBSEwuYPW tXHOEF7E+Droo0GtxN9pu1n87xGwayZW1Mhip2Qd2zNjejBtG09IRKMzCPgrpghvPbuK OV0Mupx1ofHipTv/gjDK7Pn5A8M1evnR4YImUUlvVU6u0Y1uze3/o20lWsg97LkxwS6v Pkbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699916409; x=1700521209; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=E3N8WVoRLOkhjBrZv4IOmgMW6wphPHeM3tdZaQUJCDA=; b=P0XqKqe96IsRSmgqqSBf3Iqe8czUSgIkgO+fy3hMtbLQ/y8PcIHhw9UDx2zjvcgl7D 7CDxNpI5G4kj6TIkh7rz2c/ut1IsB3NgZ9K91XwyY+YQczXTpZ9WlWkH60oUE+eNinuE Nkbj+w3RjDdYs5w9hAmXtG+r2mAwoM01qC0/ycl538bV1CYaVt2k4fbJlRw7OyQ7m3iM x6P/CnZFyDxTUepPoeXp3Tsz3jCT06jw5BkIj0g4CNSrWmRF9sC2NVsDNEW3919z9lSR JA7bXJKao6dHgb+VL3MMaSFdhwJuBZ1lSV6PjbD9cpdkYW9s0OrBkRUHEd0dBhkb1g2l LPMw== X-Gm-Message-State: AOJu0Yxc9SstBd5GKhDG8USM/NQg7jrT+/aWMnebaGg+2Ja1lzg42igC re7C8i/HSNTcNNT4PY8KNdX6RzYmm3SytjkjgmpKTxDC4Kq8IQYM83yK3fb87PCqdsD1PMUStu6 YAwgnfo4S2jU5DASQI98dpvJ1iY+r62JLmJ1qDbeaCzNsSDJizn7RaEIJfM8/mYPOR+fMdEyC3i vaaRglvb5V X-Google-Smtp-Source: AGHT+IFusA4mF/hZh2tJVABJsGETayafIZ/DUZUHxUochAoQeeoZ/d6IpzZwzcPj9+gH5sv2LHtFkuhDez1CJRslUQ== X-Received: from elbrus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:12e9]) (user=ppluzhnikov job=sendgmr) by 2002:a05:6a02:490:b0:5bd:64f8:ca66 with SMTP id bw16-20020a056a02049000b005bd64f8ca66mr140688pgb.1.1699916409105; Mon, 13 Nov 2023 15:00:09 -0800 (PST) Date: Mon, 13 Nov 2023 22:58:36 +0000 Mime-Version: 1.0 X-Mailer: git-send-email 2.42.0.869.gea05f2083d-goog Message-ID: <20231113225835.4083255-2-ppluzhnikov@google.com> Subject: [PATCH] Fix computations with (potentially) NULL pointer From: Paul Pluzhnikov <ppluzhnikov@google.com> To: elfutils-devel@sourceware.org Cc: nafi@google.com, maennich@google.com, Paul Pluzhnikov <ppluzhnikov@google.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-20.1 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL 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: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Elfutils-devel mailing list <elfutils-devel.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/elfutils-devel/> List-Post: <mailto:elfutils-devel@sourceware.org> List-Help: <mailto:elfutils-devel-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=subscribe> Errors-To: elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org |
Series |
Fix computations with (potentially) NULL pointer
|
|
Commit Message
Paul Pluzhnikov
Nov. 13, 2023, 10:58 p.m. UTC
When map_address is NULL, computing map_address+offset is technically
undefined behavior, and triggers Clang/LLVM warning when using
-fsanitize=pointer-overflow.
Fix this by using uintptr_t to perform computations.
Signed-off-by: Shahriar "Nafi" Rouf <nafi@google.com>
---
libelf/elf_begin.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Comments
Hi Paul, Hi Nafi, On Mon, 2023-11-13 at 22:58 +0000, Paul Pluzhnikov wrote: > When map_address is NULL, computing map_address+offset is technically > undefined behavior, and triggers Clang/LLVM warning when using > -fsanitize=pointer-overflow. Urgh, I had no idea NULL + ... was technically undefined behavior. > Fix this by using uintptr_t to perform computations. I can see how this solves the the issue. It would be slightly nicer if we could just do the computation after checking map_address != NULL (since ehdr is only used after such a check). That would require rearranging some of the if statements. Does that make the code too complicated? Also this only resolves the issue for the 64bit ELF case. Just above this code is basically the same code for 32bit ELF. That code also needs to be fixed. Thanks, Mark
Mark, On Tue, Nov 14, 2023 at 4:57 AM Mark Wielaard <mark@klomp.org> wrote: > Urgh, I had no idea NULL + ... was technically undefined behavior. ISO/IEC 9899:201x 6.5.6p8 When an expression that has integer type is added to or subtracted from a pointer, the result has the type of the pointer operand. If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integer expression. ... If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined. > It would be slightly nicer if > we could just do the computation after checking map_address != NULL > (since ehdr is only used after such a check). That would require > rearranging some of the if statements. Does that make the code too > complicated? I tried it, and it does: we need both "map_addr != 0" and "ehdr is properly aligned", but we can't compute the latter before evaluating the former, and we have the else clause when either condition fails. I can fix this with a goto, or a helper variable, but the current solution of keeping ehdr as uintptr_t is much simpler. It also reduces the casting and line wrapping required :-) > Also this only resolves the issue for the 64bit ELF case. Just above > this code is basically the same code for 32bit ELF. That code also > needs to be fixed. Sorry I missed that. Revised patch attached. Thanks,
Hi Paul, On Tue, 2023-11-14 at 08:12 -0800, Paul Pluzhnikov wrote: > On Tue, Nov 14, 2023 at 4:57 AM Mark Wielaard <mark@klomp.org> wrote: > > > Urgh, I had no idea NULL + ... was technically undefined behavior. > > ISO/IEC 9899:201x > 6.5.6p8 > > When an expression that has integer type is added to or subtracted > from a pointer, the result has the type of the pointer operand. If the > pointer operand points to an element of an array object, and the array > is large enough, the result points to an element offset from the > original element such that the difference of the subscripts of the > resulting and original array elements equals the integer expression. > ... If both the pointer operand and the result point to elements of > the same array object, or one past the last element of the array > object, the evaluation shall not produce an overflow; otherwise, the > behavior is undefined. Aha, so since NULL isn't a pointer to an array object a compiler may assume the pointer isn't NULL and possibly remove any NULL checks later in the code if it sees you do pointer arithmetic... boo. > > It would be slightly nicer if > > we could just do the computation after checking map_address != NULL > > (since ehdr is only used after such a check). That would require > > rearranging some of the if statements. Does that make the code too > > complicated? > > I tried it, and it does: we need both "map_addr != 0" and "ehdr is > properly aligned", but we can't compute the latter before evaluating > the former, and we have the else clause when either condition fails. I > can fix this with a goto, or a helper variable, but the current > solution of keeping ehdr as uintptr_t is much simpler. > > It also reduces the casting and line wrapping required :-) Yeah, I was afraid of that. Thanks for looking into it though. > > Also this only resolves the issue for the 64bit ELF case. Just above > > this code is basically the same code for 32bit ELF. That code also > > needs to be fixed. > > Sorry I missed that. I am slightly surprised our testsuite didn't catch this. We do have -- enable-sanitize-undefined which does build everything with -- sanitize=undefined. Which should enable -fsanitize=pointer-overflow. But I just tried (with gcc) and it seems to not trigger. > Revised patch attached. Looks good. Applied. Thanks, Mark
Mark, On Tue, Nov 14, 2023 at 8:57 AM Mark Wielaard <mark@klomp.org> wrote: > I am slightly surprised our testsuite didn't catch this. We do have -- > enable-sanitize-undefined which does build everything with -- > sanitize=undefined. Which should enable -fsanitize=pointer-overflow. > But I just tried (with gcc) and it seems to not trigger. This was exposed by Clang (and a close to HEAD Clang at that -- I am not sure whether released Clang also catches this). > Looks good. Applied. Thanks! Appreciate the speedy reviews.
Hi Paul, On Tue, 2023-11-14 at 09:04 -0800, Paul Pluzhnikov wrote: > On Tue, Nov 14, 2023 at 8:57 AM Mark Wielaard <mark@klomp.org> wrote: > > Looks good. Applied. > > Thanks! > > Appreciate the speedy reviews. Unfortunately our 32bit buildbots were also very quick to point out an issue: https://builder.sourceware.org/buildbot/#/changes/35202 elfutils-debian-i386 and elfutils-debian-armhf fail to compile the same way: elf_begin.c: In function ‘file_read_elf’: elf_begin.c:495:30: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + e_shoff); ^ cc1: all warnings being treated as errors This is I think because e_shoff is an Elf64_Off, so (ehdr + e_shoff) gets promoted to 64bits... Which does expose an interesting issue that (theoretically) mmaped 64bit Elf files cannot be used on 32bit architectures... hohum. I don't immediately know what the correct fix is here. I'll think about it over dinner. Cheers, Mark
On Tue, Nov 14, 2023 at 9:55 AM Mark Wielaard <mark@klomp.org> wrote: > Unfortunately our 32bit buildbots were also very quick to point out an > issue: https://builder.sourceware.org/buildbot/#/changes/35202 Sorry about the break. I just tried "./configure "CC=gcc -m32" "CXX=g++ -m32" and that didn't reproduce the failure. > Which does expose an interesting issue that (theoretically) mmaped > 64bit Elf files cannot be used on 32bit architectures... hohum. The failure here would be when map_addr ends up high in memory, and e_shoff is so large that it causes a wrap around. Section headers do tend to be near the end of the ELF. One of our large 64-bit binaries (3.6GiB in size) has e_shoff == 3803727624, so the overflow seems very likely here ... except the mmap would have failed already, because the mmap covers the entire file. So I think the overflow can not happen in practice. If that's true, we can cast e_shoff to ptrdiff_t to suppress the warning. diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c index 9f8196b6..dcaad8ee 100644 --- a/libelf/elf_begin.c +++ b/libelf/elf_begin.c @@ -492,7 +492,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, goto free_and_out; if (scncnt > 0) - elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + e_shoff); + elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + (ptrdiff_t) e_shoff); for (size_t cnt = 0; cnt < scncnt; ++cnt) {
Hi Paul, On Tue, Nov 14, 2023 at 10:56:50AM -0800, Paul Pluzhnikov wrote: > On Tue, Nov 14, 2023 at 9:55 AM Mark Wielaard <mark@klomp.org> wrote: > > > Unfortunately our 32bit buildbots were also very quick to point out an > > issue: https://builder.sourceware.org/buildbot/#/changes/35202 > > Sorry about the break. No worries. That is what the bots are for :) > I just tried "./configure "CC=gcc -m32" "CXX=g++ -m32" and that didn't > reproduce the failure. Are you sure? It does for me. It also confirmed your suggestion below fixes it. > > Which does expose an interesting issue that (theoretically) mmaped > > 64bit Elf files cannot be used on 32bit architectures... hohum. > > The failure here would be when map_addr ends up high in memory, and > e_shoff is so large that it causes a wrap around. > > Section headers do tend to be near the end of the ELF. One of our > large 64-bit binaries (3.6GiB in size) has e_shoff == 3803727624, so > the overflow seems very likely here ... except the mmap would have > failed already, because the mmap covers the entire file. So I think > the overflow can not happen in practice. O, good observation. You are right. > If that's true, we can cast e_shoff to ptrdiff_t to suppress the warning. Thanks. I pushed that fix as attached. Cheers, Mark From f84f1cd7e296bf223cb45b5469978d4bea82cec0 Mon Sep 17 00:00:00 2001 From: Mark Wielaard <mark@klomp.org> Date: Tue, 14 Nov 2023 21:34:50 +0100 Subject: [PATCH] libelf: Fix elf_begin.c build on 32bit arches. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 32bit architectures gcc produces an error: elf_begin.c: In function ‘file_read_elf’: elf_begin.c:495:30: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + e_shoff); ^ This is because we are adding an uintptr to an Elf64_Off which promotes the result to a 64bit value. Fix this by casting the e_shoff to an ptrdiff_t. This is fine since the mmap of the file would have failed if it didn't fit in the 32bit address space and we check that e_shoff fits inside the image. * libelf/elf_begin.c (file_read_elf): Cast e_shoff to ptrdiff_t before adding to ehdr. Suggested-by: Paul Pluzhnikov <ppluzhnikov@google.com> Signed-off-by: Mark Wielaard <mark@klomp.org> --- libelf/elf_begin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c index 9f8196b6..dcaad8ee 100644 --- a/libelf/elf_begin.c +++ b/libelf/elf_begin.c @@ -492,7 +492,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, goto free_and_out; if (scncnt > 0) - elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + e_shoff); + elf->state.elf64.shdr = (Elf64_Shdr *) (ehdr + (ptrdiff_t) e_shoff); for (size_t cnt = 0; cnt < scncnt; ++cnt) {
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c index fe8c640a..da495bef 100644 --- a/libelf/elf_begin.c +++ b/libelf/elf_begin.c @@ -445,15 +445,15 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, { /* This pointer might not be directly usable if the alignment is not sufficient for the architecture. */ - Elf64_Ehdr *ehdr = (Elf64_Ehdr *) ((char *) map_address + offset); + uintptr_t ehdr = (uintptr_t) map_address + offset; /* This is a 64-bit binary. */ if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA && (ALLOW_UNALIGNED - || (((uintptr_t) ehdr) & (__alignof__ (Elf64_Ehdr) - 1)) == 0)) + || (ehdr & (__alignof__ (Elf64_Ehdr) - 1)) == 0)) { /* We can use the mmapped memory. */ - elf->state.elf64.ehdr = ehdr; + elf->state.elf64.ehdr = (Elf64_Ehdr *) ehdr; } else { @@ -486,7 +486,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, if (map_address != NULL && e_ident[EI_DATA] == MY_ELFDATA && cmd != ELF_C_READ_MMAP /* We need a copy to be able to write. */ && (ALLOW_UNALIGNED - || ((((uintptr_t) ehdr + e_shoff) + || (((ehdr + e_shoff) & (__alignof__ (Elf64_Shdr) - 1)) == 0))) { if (unlikely (scncnt > 0 && e_shoff >= maxsize) @@ -496,7 +496,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, if (scncnt > 0) elf->state.elf64.shdr - = (Elf64_Shdr *) ((char *) ehdr + e_shoff); + = (Elf64_Shdr *) (ehdr + e_shoff); for (size_t cnt = 0; cnt < scncnt; ++cnt) {