diff mbox series

[V6,1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]

Message ID 20230328154944.3946619-2-qing.zhao@oracle.com
State New
Headers
Series Handle component_ref to a structure/union field including FAM for builtin_object_size |

Commit Message

Qing Zhao March 28, 2023, 3:49 p.m. UTC 
  the C front-end has been approved by Joseph.

Jacub, could you please eview the middle end part of the changes of this patch?

The major change is in tree-object-size.cc (addr_object_size).
 (To use the new TYPE_INCLUDE_FLEXARRAY info). 

This patch is to fix PR101832(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832),
and is needed for Linux Kernel security.  It’s better to be put into GCC13.

Thanks a lot!

Qing

==========

GCC extension accepts the case when a struct with a flexible array member
is embedded into another struct or union (possibly recursively).
__builtin_object_size should treat such struct as flexible size per
-fstrict-flex-arrays.

gcc/c/ChangeLog:

	PR tree-optimization/101832
	* c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
	struct/union type.

gcc/lto/ChangeLog:

	PR tree-optimization/101832
	* lto-common.cc (compare_tree_sccs_1): Compare bit
	TYPE_NO_NAMED_ARGS_STDARG_P or TYPE_INCLUDE_FLEXARRAY properly
	for its corresponding type.

gcc/ChangeLog:

	PR tree-optimization/101832
	* print-tree.cc (print_node): Print new bit type_include_flexarray.
	* tree-core.h (struct tree_type_common): Use bit no_named_args_stdarg_p
	as type_include_flexarray for RECORD_TYPE or UNION_TYPE.
	* tree-object-size.cc (addr_object_size): Handle structure/union type
	when it has flexible size.
	* tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
	in bit no_named_args_stdarg_p properly for its corresponding type.
	* tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
	out bit no_named_args_stdarg_p properly for its corresponding type.
	* tree.h (TYPE_INCLUDE_FLEXARRAY): New macro TYPE_INCLUDE_FLEXARRAY.

gcc/testsuite/ChangeLog:

	PR tree-optimization/101832
	* gcc.dg/builtin-object-size-pr101832.c: New test.
---
 gcc/c/c-decl.cc                               |  11 ++
 gcc/lto/lto-common.cc                         |   5 +-
 gcc/print-tree.cc                             |   5 +
 .../gcc.dg/builtin-object-size-pr101832.c     | 134 ++++++++++++++++++
 gcc/tree-core.h                               |   2 +
 gcc/tree-object-size.cc                       |  23 ++-
 gcc/tree-streamer-in.cc                       |   5 +-
 gcc/tree-streamer-out.cc                      |   5 +-
 gcc/tree.h                                    |   7 +-
 9 files changed, 192 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c

Comments

Kees Cook April 12, 2023, 6:46 p.m. UTC | #1 
On Tue, Mar 28, 2023 at 03:49:43PM +0000, Qing Zhao wrote:
> the C front-end has been approved by Joseph.
> 
> Jacub, could you please eview the middle end part of the changes of this patch?
> 
> The major change is in tree-object-size.cc (addr_object_size).
>  (To use the new TYPE_INCLUDE_FLEXARRAY info). 
> 
> This patch is to fix PR101832(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832),
> and is needed for Linux Kernel security.  It’s better to be put into GCC13.
> 
> Thanks a lot!

Just to confirm, I've done build testing with the Linux kernel, and this
is behaving as I'd expect. This makes my life MUCH easier -- many fewer
false positives for our bounds checking. :)

-Kees
Qing Zhao April 17, 2023, 12:56 p.m. UTC | #2 
Thanks a lot for the testing, Kees.

Jakub, do you have any more comment on the 6th version of the patches?
Is it ready for the GCC13 commit? Or we need to delay it to GCC14?

This patch has been there waiting for approving for a while, I think that it’s a nice improvement to GCC and also help to the
Kernel security. 

Thanks.

Qing

> On Apr 12, 2023, at 2:46 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Tue, Mar 28, 2023 at 03:49:43PM +0000, Qing Zhao wrote:
>> the C front-end has been approved by Joseph.
>> 
>> Jacub, could you please eview the middle end part of the changes of this patch?
>> 
>> The major change is in tree-object-size.cc (addr_object_size).
>> (To use the new TYPE_INCLUDE_FLEXARRAY info). 
>> 
>> This patch is to fix PR101832(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832),
>> and is needed for Linux Kernel security.  It’s better to be put into GCC13.
>> 
>> Thanks a lot!
> 
> Just to confirm, I've done build testing with the Linux kernel, and this
> is behaving as I'd expect. This makes my life MUCH easier -- many fewer
> false positives for our bounds checking. :)
> 
> -Kees
> 
> -- 
> Kees Cook
diff mbox series

Patch

diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index e537d33f398..14c54809b9d 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -9258,6 +9258,17 @@  finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
       /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x.  */
       DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
 
+      /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t.
+	 when x is an array and is the last field.  */
+      if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
+	TYPE_INCLUDE_FLEXARRAY (t)
+	  = is_last_field && flexible_array_member_type_p (TREE_TYPE (x));
+      /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
+	 when x is an union or record and is the last field.  */
+      else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
+	TYPE_INCLUDE_FLEXARRAY (t)
+	  = is_last_field && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x));
+
       if (DECL_NAME (x)
 	  || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
 	saw_named_field = true;
diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc
index 882dd8971a4..9dde7118266 100644
--- a/gcc/lto/lto-common.cc
+++ b/gcc/lto/lto-common.cc
@@ -1275,7 +1275,10 @@  compare_tree_sccs_1 (tree t1, tree t2, tree **map)
       if (AGGREGATE_TYPE_P (t1))
 	compare_values (TYPE_TYPELESS_STORAGE);
       compare_values (TYPE_EMPTY_P);
-      compare_values (TYPE_NO_NAMED_ARGS_STDARG_P);
+      if (FUNC_OR_METHOD_TYPE_P (t1))
+	compare_values (TYPE_NO_NAMED_ARGS_STDARG_P);
+      if (RECORD_OR_UNION_TYPE_P (t1))
+	compare_values (TYPE_INCLUDE_FLEXARRAY);
       compare_values (TYPE_PACKED);
       compare_values (TYPE_RESTRICT);
       compare_values (TYPE_USER_ALIGN);
diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
index 1f3afcbbc86..efacdb7686f 100644
--- a/gcc/print-tree.cc
+++ b/gcc/print-tree.cc
@@ -631,6 +631,11 @@  print_node (FILE *file, const char *prefix, tree node, int indent,
 	  && TYPE_CXX_ODR_P (node))
 	fputs (" cxx-odr-p", file);
 
+      if ((code == RECORD_TYPE
+	   || code == UNION_TYPE)
+	  && TYPE_INCLUDE_FLEXARRAY (node))
+	fputs (" include-flexarray", file);
+
       /* The transparent-union flag is used for different things in
 	 different nodes.  */
       if ((code == UNION_TYPE || code == RECORD_TYPE)
diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
new file mode 100644
index 00000000000..60078e11634
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
@@ -0,0 +1,134 @@ 
+/* PR 101832: 
+   GCC extension accepts the case when a struct with a C99 flexible array
+   member is embedded into another struct (possibly recursively).
+   __builtin_object_size will treat such struct as flexible size.
+   However, when a structure with non-C99 flexible array member, i.e, trailing
+   [0], [1], or [4], is embedded into anther struct, the stucture will not
+   be treated as flexible size.  */ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#include "builtin-object-size-common.h"
+
+#define expect(p, _v) do { \
+  size_t v = _v; \
+  if (p == v) \
+    __builtin_printf ("ok:  %s == %zd\n", #p, p); \
+  else {\
+    __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+    FAIL (); \
+  } \
+} while (0);
+
+
+struct A {
+  int n;
+  char data[];
+};
+
+struct B {
+  int m;
+  struct A a;
+};
+
+struct C {
+  int q;
+  struct B b;
+};
+
+struct A0 {
+  int n;
+  char data[0];
+};
+
+struct B0 {
+  int m;
+  struct A0 a;
+};
+
+struct C0 {
+  int q;
+  struct B0 b;
+};
+
+struct A1 {
+  int n;
+  char data[1];
+};
+
+struct B1 {
+  int m;
+  struct A1 a;
+};
+
+struct C1 {
+  int q;
+  struct B1 b;
+};
+
+struct An {
+  int n;
+  char data[8];
+};
+
+struct Bn {
+  int m;
+  struct An a;
+};
+
+struct Cn {
+  int q;
+  struct Bn b;
+};
+
+volatile void *magic1, *magic2;
+
+int main (int argc, char *argv[])
+{
+  struct B *outer;
+  struct C *outest;
+
+  /* Make sure optimization can't find some other object size. */
+  outer = (void *)magic1;
+  outest = (void *)magic2;
+
+  expect (__builtin_object_size (&outer->a, 1), -1);
+  expect (__builtin_object_size (&outest->b, 1), -1);
+  expect (__builtin_object_size (&outest->b.a, 1), -1);
+
+  struct B0 *outer0;
+  struct C0 *outest0;
+
+  /* Make sure optimization can't find some other object size. */
+  outer0 = (void *)magic1;
+  outest0 = (void *)magic2;
+
+  expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
+  expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
+  expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
+
+  struct B1 *outer1;
+  struct C1 *outest1;
+
+  /* Make sure optimization can't find some other object size. */
+  outer1 = (void *)magic1;
+  outest1 = (void *)magic2;
+
+  expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
+  expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
+  expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
+
+  struct Bn *outern;
+  struct Cn *outestn;
+
+  /* Make sure optimization can't find some other object size. */
+  outern = (void *)magic1;
+  outestn = (void *)magic2;
+
+  expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
+  expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
+  expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
+
+  DONE ();
+  return 0;
+}
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index fd2be57b78c..83482537a6d 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1717,6 +1717,8 @@  struct GTY(()) tree_type_common {
   unsigned typeless_storage : 1;
   unsigned empty_flag : 1;
   unsigned indivisible_p : 1;
+  /* TYPE_NO_NAMED_ARGS_STDARG_P for a stdarg function.
+     Or TYPE_INCLUDE_FLEXARRAY for RECORD_TYPE and UNION_TYPE.  */
   unsigned no_named_args_stdarg_p : 1;
   unsigned spare : 15;
 
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 9a936a91983..1619d144ecd 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -633,11 +633,32 @@  addr_object_size (struct object_size_info *osi, const_tree ptr,
 		    v = NULL_TREE;
 		    break;
 		  case COMPONENT_REF:
-		    if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
+		    /* When the ref is not to an aggregate type, i.e, an array,
+		       a record or a union, it will not have flexible size,
+		       compute the object size directly.  */
+		    if (!AGGREGATE_TYPE_P (TREE_TYPE (v)))
 		      {
 			v = NULL_TREE;
 			break;
 		      }
+		    /* if the ref is to a record or union type, but the type
+		       does not include a flexible array recursively, compute
+		       the object size directly.  */
+		    if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (v)))
+		      {
+			if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
+			  {
+			    v = NULL_TREE;
+			    break;
+			  }
+			else
+			  {
+			    v = TREE_OPERAND (v, 0);
+			    break;
+			  }
+		      }
+		    /* Now the ref is to an array type.  */
+		    gcc_assert (TREE_CODE (TREE_TYPE (v)) == ARRAY_TYPE);
 		    is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
 		    while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
 		      if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
index d4dc30f048f..56add06a70a 100644
--- a/gcc/tree-streamer-in.cc
+++ b/gcc/tree-streamer-in.cc
@@ -398,7 +398,10 @@  unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
   if (AGGREGATE_TYPE_P (expr))
     TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_EMPTY_P (expr) = (unsigned) bp_unpack_value (bp, 1);
-  TYPE_NO_NAMED_ARGS_STDARG_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+  if (FUNC_OR_METHOD_TYPE_P (expr))
+    TYPE_NO_NAMED_ARGS_STDARG_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+  if (RECORD_OR_UNION_TYPE_P (expr))
+    TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp);
   SET_TYPE_ALIGN (expr, bp_unpack_var_len_unsigned (bp));
 #ifdef ACCEL_COMPILER
diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
index d107229da5c..4a1d481015b 100644
--- a/gcc/tree-streamer-out.cc
+++ b/gcc/tree-streamer-out.cc
@@ -365,7 +365,10 @@  pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
   if (AGGREGATE_TYPE_P (expr))
     bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1);
   bp_pack_value (bp, TYPE_EMPTY_P (expr), 1);
-  bp_pack_value (bp, TYPE_NO_NAMED_ARGS_STDARG_P (expr), 1);
+  if (FUNC_OR_METHOD_TYPE_P (expr))
+    bp_pack_value (bp, TYPE_NO_NAMED_ARGS_STDARG_P (expr), 1);
+  if (RECORD_OR_UNION_TYPE_P (expr))
+    bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
   bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr));
   bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr));
 }
diff --git a/gcc/tree.h b/gcc/tree.h
index abcdb5638d4..3ab3a3a0cc7 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -786,7 +786,12 @@  extern void omp_clause_range_check_failed (const_tree, const char *, int,
    (...) prototype, where arguments can be accessed with va_start and
    va_arg), as opposed to an unprototyped function.  */
 #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
-  (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
+  (FUNC_OR_METHOD_CHECK (NODE)->type_common.no_named_args_stdarg_p)
+
+/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
+   at the last field recursively.  */
+#define TYPE_INCLUDE_FLEXARRAY(NODE) \
+  (RECORD_OR_UNION_CHECK (NODE)->type_common.no_named_args_stdarg_p)
 
 /* In an IDENTIFIER_NODE, this means that assemble_name was called with
    this string as an argument.  */