From patchwork Wed Nov 1 09:39:16 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Uecker X-Patchwork-Id: 1857869 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=tugraz.at header.i=@tugraz.at header.a=rsa-sha256 header.s=mailrelay header.b=HkDnMg9r; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4SL26K0H6Tz1yQm for ; Wed, 1 Nov 2023 20:39:50 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2DF9D385840D for ; Wed, 1 Nov 2023 09:39:48 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mailrelay.tugraz.at (mailrelay.tugraz.at [129.27.2.202]) by sourceware.org (Postfix) with ESMTPS id AE66A3858D35 for ; Wed, 1 Nov 2023 09:39:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AE66A3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=tugraz.at Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tugraz.at ARC-Filter: OpenARC Filter v1.0.0 sourceware.org AE66A3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=129.27.2.202 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698831576; cv=none; b=k5fRPkZU5Ly80zTO5jGohVH3VW9jXkXnZZ/GAeZlNux95JkoIL6ldGOLd+ohTWsEDBn3AL4SCBcAQtao30Ov4PORVwKq5svS65UF4aatYKXy2M9ZF9lgfAby/QRhGN7LtwbpEA3f0fXgDqLOlBq8OpjTQMOn03pswewW93IKRBU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698831576; c=relaxed/simple; bh=oRu5cjd2/7fgVfw+0q5LAZOKwjfztx8v8W5iVhvKDi0=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=VRA3CcYXFy4bo+//7HEKPn1rH5hcw0ohqQPiroC9d8dlchyokptPA1Ks9dC1f9+NI4RO10YTDBzg4Nqjw2VFL6iIPJpotPsMvLlqTFm4TKsG14fuKKBNRjO/KB4cTHSgvAKbeb5LLgUMUzX8delZ3SqK7hVHf29Galq1ILafd70= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from vra-169-216.tugraz.at (vra-169-216.tugraz.at [129.27.169.216]) by mailrelay.tugraz.at (Postfix) with ESMTPSA id 4SL25c3S59z1HNLS; Wed, 1 Nov 2023 10:39:16 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 mailrelay.tugraz.at 4SL25c3S59z1HNLS DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tugraz.at; s=mailrelay; t=1698831557; bh=ZIGkjARmOjCBZnr/9jnPknPFrS7oofV1SWsckuMELyw=; h=Subject:From:To:Cc:Date:From; b=HkDnMg9rAN7+Avxse6Uvmrb8Gg27GKqN70w1pShQN+f3YqoEBG4xwhNxRx7CDFkNg B1/2JxQ8tPYA2ygXlKeK0akyftkPD9DzTVoBFyiyCXv016EVJVqZiG+h9rynAkBk+z 5/no3EAU5cYtmkYmBdYOsFIpVHq+YOUdSGQvUhn8= Message-ID: <01ed76d1286383f7a7e0a378be6c82bec12a2fd8.camel@tugraz.at> Subject: RFC [PATCH] c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608] From: Martin Uecker To: gcc-patches@gcc.gnu.org Cc: Joseph Myers , Marek Polacek , Jakub Jelinek Date: Wed, 01 Nov 2023 10:39:16 +0100 User-Agent: Evolution 3.46.4-2 MIME-Version: 1.0 X-TUG-Backscatter-control: G/VXY7/6zeyuAY/PU2/0qw X-Spam-Scanner: SpamAssassin 3.003001 X-Spam-Score-relay: -1.9 X-Scanned-By: MIMEDefang 2.74 on 129.27.10.116 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Here is a patch that adds the missing cases for vla size instrumentation. This now includes all cases where a type with size < 0 is created, which is already UB and not just cases where a VLA is allocated. But a VLA can be allocated based on an typedef, which is also now indirectly protected in this way. I moved the instrumentation from the size itself as its own term into  the expression that evaluate size expressions for side effects. This avoids confusing other warning code that looks at the size expressions (-Wvla-parameter). There is one open question though: How to to treat n == 0?  Here I preliminary changed this to n > 0 (also for the existing case), because when also detecting n == 0 this tools especially when instrumenting all the types becomes basically useless because of  the very common (and unproblematic) use of n == 0.   But strictly speaking n == 0 is also UB and as pointed out in  PR98609 the error message is then not entirely accurate because it says non-positive and not negative. I do not think it is confusing though because it is still always correct. One could consider splitting it up into vla-bound / vla-bound-strict, but changing the error message would require further upstream changes and dealing with this far exceeds the time I can afford contributing to this this. Another complication is that we ran out of bits for sanitizer flags in unsigned int, so this would also require more changes. Any advice? I think it would be important to have complete UBSan coverage for all size and bounds issues related to VM types and it would be nice to get this in GCC 14. (I find this extremely useful in my projects). Martin c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608] Add vla-bound instrumentation for all VLAs including VLAs in parameters and fields, but allow zero-sized errors. Bootstrapped and regression tested on x86. PR c/98608 gcc/c: * c-decl.cc (grokdeclarator): Instrument all VLAs. gcc/c-family: * c-ubsan.cc (ubsan_instrument_vla): Do not include zero length. gcc/testsuite: * gcc.dg/ubsan/pr89608.c: New test. * gcc.dg/ubsan/vla-1.c: New test. * gcc.dg/ubsan/vla-2.c: New test. * gcc.dg/ubsan/vla-3.c: New test. * c-c++-common/ubsan/vla-1.c: Adapt. diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc index b2c58c65d97..8983ede0166 100644 --- a/gcc/c-family/c-ubsan.cc +++ b/gcc/c-family/c-ubsan.cc @@ -313,7 +313,7 @@ ubsan_instrument_vla (location_t loc, tree size) tree type = TREE_TYPE (size); tree t, tt; - t = fold_build2 (LE_EXPR, boolean_type_node, size, build_int_cst (type, 0)); + t = fold_build2 (LT_EXPR, boolean_type_node, size, build_int_cst (type, 0)); if (flag_sanitize_trap & SANITIZE_VLA) tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0); else diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index 7a145bed281..752a65d6729 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -7201,16 +7201,20 @@ grokdeclarator (const struct c_declarator *declarator, with known value. */ this_size_varies = size_varies = true; warn_variable_length_array (name, size); - if (sanitize_flags_p (SANITIZE_VLA) - && current_function_decl != NULL_TREE - && decl_context == NORMAL) + if (sanitize_flags_p (SANITIZE_VLA)) { /* Evaluate the array size only once. */ size = save_expr (size); size = c_fully_fold (size, false, NULL); - size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size), - ubsan_instrument_vla (loc, size), - size); + tree instr = ubsan_instrument_vla (loc, size); + /* We have to build this in the right order, so + instrumentation is done before the size can + be used in other parameters. */ + if (*expr) + *expr = build2 (COMPOUND_EXPR, TREE_TYPE (instr), + *expr, instr); + else + *expr = instr; } } diff --git a/gcc/testsuite/c-c++-common/ubsan/vla-1.c b/gcc/testsuite/c-c++-common/ubsan/vla-1.c index c97465edae1..cc441ffc80b 100644 --- a/gcc/testsuite/c-c++-common/ubsan/vla-1.c +++ b/gcc/testsuite/c-c++-common/ubsan/vla-1.c @@ -110,9 +110,7 @@ main (void) /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -5\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -3\[^\n\r]*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -6\[^\n\r]*(\n|\r\n|\r)" } */ diff --git a/gcc/testsuite/gcc.dg/ubsan/pr98608.c b/gcc/testsuite/gcc.dg/ubsan/pr98608.c new file mode 100644 index 00000000000..9f8f40c9643 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/pr98608.c @@ -0,0 +1,16 @@ +/* PR 98608 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound" } */ + +void f(int n, double (*x)[n]) +{ + typeof(*x) y; +} + +int main() +{ + f(-1, 0); +} + +/* { dg-output "variable length array bound evaluates to non-positive value -1" } */ + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-1.c b/gcc/testsuite/gcc.dg/ubsan/vla-1.c new file mode 100644 index 00000000000..6544698caf9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-1.c @@ -0,0 +1,27 @@ +/* { dg-do run } */ +/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */ + +void f1(int n, char buf[n]) { } +/* { dg-output "variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f2(int n, float m[n][2]) { } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f3(int n, int m, float x[n][m]) { } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f4(int n) { typedef char buf[n]; } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f5(int n) { struct { char buf[n]; } x; } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f6(int n, struct { char buf[n]; } *p) { } /* { dg-warning "anonymous" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1" } */ + +int main() +{ + f1(-1, 0); + f2(-1, 0); + f3(-1, 1, 0); + f4(-1); + f5(-1); + f6(-1, 0); +} + + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-2.c b/gcc/testsuite/gcc.dg/ubsan/vla-2.c new file mode 100644 index 00000000000..5937f860e89 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-2.c @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */ + +void f1(int n, char buf[n]) { } +void f2(int n, float m[n][2]) { } +void f3(int n, int m, float x[n][m]) { } +void f4(int n) { typedef char buf[n]; } +void f5(int n) { struct { char buf[n]; } x; } +void f6(int n, struct { char buf[n]; } *p) { } /* { dg-warning "anonymous" } */ + +int main() +{ + f1(0, 0); + f2(0, 0); + f3(0, 1, 0); + f4(0); + f5(0); + f6(0, 0); +} + + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-3.c b/gcc/testsuite/gcc.dg/ubsan/vla-3.c new file mode 100644 index 00000000000..9f4b0555328 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-3.c @@ -0,0 +1,14 @@ +/* { dg-do run } + * { dg-options "-fsanitize=vla-bound" } */ + +void foo(int n, char (*buf)[n], char p[n = 1]) +{ +} + +int main() +{ + foo(-1, 0, 0); +} + +/* { dg-output "variable length array bound evaluates to non-positive value -1" } */ +