On Thu, 2023-03-16 at 19:25 +0100, Hans-Peter Nilsson wrote: > > From: David Malcolm <dmalc...@redhat.com> > > Date: Thu, 16 Mar 2023 13:55:48 -0400 > > > On Thu, 2023-03-09 at 19:56 +0100, Hans-Peter Nilsson wrote: > > > It's not obvious to me whether considered best to include or > > > exclude these tests that depend on structure layout details. > > > If excluding, the obvious alternative to this patch is then > > > to add a top one-liner (to dg-skip-if the test for > > > default_packed targets or a similar excluding expression). > > > I'm fine either way, just suggesting the following, which > > > handles the cris-elf test-case failures I see for these > > > tests, and causes no change in results for native > > > x86_64-pc-linux-gnu. > > > > Thanks for looking at this. > > > > How about a third option: can the structs be explicitly marked as > > being > > packed, by adding __attribute__((__packed__)) to the various > > structs? > > The tests are all about detecting problems with padding bits, and > > presumably we can have padding bits on all targets if we explicitly > > ask > > for them. > > > > Does that make for a simpler patch? > > Did I get you right: making the layout the same for all > targets, by -for all structs that in my patch needed > different layout- marking them with > __attribute__((__packed__)) and adjust numbers in warnings? > > That doesn't seem like a good idea. At a glance the > *testcode* will be simpler, but the patch will be slightly > larger and have a lot of "-" lines instead of "+" lines, as > the patch cause a lot of warnings to be dropped: you'll test > for absence of warnings instead of proper warnings. > > Looks like you'll lose 24 of the padding tests; 30 lines > where I added "target ! default_packed" and 6 where I added > "target default_packed". > > Perhaps I misunderstood?
No, I think I'm misunderstanding the problem; sorry. I think I prefer the top one-liner dg-skip-if approach you mentioned in your original email; it seems simplest. Thanks Dave > > brgds, H-P > > > > Dave > > > > > > > > Beware that some of the tests have lines with trailing > > > whitespace. Where lines are changed in this patch, the > > > trailing whitespace is removed. > > > > > > > > > > Ok to commit? > > > > > > -- >8 -- > > > It's a judgement call whether to just skip some of these > > > tests rather than trying to match messages depending on the > > > layout of structures, but better include than exclude. > > > > > > * gcc.dg/plugin/infoleak-2.c, > > > gcc.dg/plugin/infoleak-CVE-2011-1078-1.c, > > > gcc.dg/plugin/infoleak-CVE-2011-1078-2.c, > > > gcc.dg/plugin/infoleak-CVE-2017-18549-1.c, > > > gcc.dg/plugin/infoleak-CVE-2017-18550-1.c, > > > gcc.dg/plugin/infoleak-antipatterns-1.c, > > > gcc.dg/plugin/infoleak-fixit-1.c: Handle default_packed > > > targets. > > > --- > > > gcc/testsuite/gcc.dg/plugin/infoleak-2.c | 13 > > > ++++++++--- > > > -- > > > .../gcc.dg/plugin/infoleak-CVE-2011-1078-1.c | 10 +++++-- > > > --- > > > .../gcc.dg/plugin/infoleak-CVE-2011-1078-2.c | 10 +++++-- > > > --- > > > .../gcc.dg/plugin/infoleak-CVE-2017-18549-1.c | 10 +++++-- > > > --- > > > .../gcc.dg/plugin/infoleak-CVE-2017-18550-1.c | 7 ++++--- > > > .../gcc.dg/plugin/infoleak-antipatterns-1.c | 10 +++++-- > > > --- > > > gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c | 10 ++++++- > > > --- > > > 7 files changed, 38 insertions(+), 32 deletions(-) > > > > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-2.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-2.c > > > index 252f8f25918a..4ba484b3c6be 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-2.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-2.c > > > @@ -18,16 +18,19 @@ struct st > > > int b:1; /* { dg-message "field 'b' is uninitialized \\(1 > > > bit\\)" > > > "field" } */ > > > /* { dg-message "padding after field 'b' is > > > uninitialized > > > \\(7 bits\\)" "padding" { target *-*-* } .-1 } */ > > > u8 d; /* { dg-message "field 'd' is uninitialized \\(1 > > > byte\\)" > > > } */ > > > - int c:7; /* { dg-message "padding after field 'c' is > > > uninitialized > > > \\(9 bits\\)" } */ > > > - u16 e; /* { dg-message "padding after field 'e' is > > > uninitialized > > > \\(2 bytes\\)" } */ > > > + int c:7; /* { dg-message "padding after field 'c' is > > > uninitialized > > > \\(9 bits\\)" "padding" { target { ! default_packed } } } */ > > > + /* { dg-message "padding after field 'c' is > > > uninitialized > > > \\(1 bit\\)" "padding" { target default_packed } .-1 } */ > > > + u16 e; /* { dg-message "padding after field 'e' is > > > uninitialized > > > \\(2 bytes\\)" "padding" { target { ! default_packed } } } */ > > > }; > > > > > > void test (void __user *dst, u16 v) > > > { > > > struct st s; /* { dg-message "region created on stack here" > > > "where" } */ > > > - /* { dg-message "capacity: 12 bytes" "capacity" { target *-*-* > > > } > > > .-1 } */ > > > - /* { dg-message "suggest forcing zero-initialization by > > > providing > > > a '\\{0\\}' initializer" "fix-it" { target *-*-* } .-2 } */ > > > + /* { dg-message "capacity: 12 bytes" "capacity" { target { ! > > > default_packed } } .-1 } */ > > > + /* { dg-message "capacity: 9 bytes" "capacity" { target > > > default_packed } .-2 } */ > > > + /* { dg-message "suggest forcing zero-initialization by > > > providing > > > a '\\{0\\}' initializer" "fix-it" { target *-*-* } .-3 } */ > > > s.e = v; > > > copy_to_user(dst, &s, sizeof (struct st)); /* { dg-warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" "warning" } */ > > > - /* { dg-message "10 bytes are uninitialized" "note how much" { > > > target *-*-* } .-1 } */ > > > + /* { dg-message "10 bytes are uninitialized" "note how much" { > > > target { ! default_packed } } .-1 } */ > > > + /* { dg-message "7 bytes are uninitialized" "note how much" { > > > target default_packed } .-2 } */ > > > } > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078- > > > 1.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c > > > index 3616fbe176b3..9269b911b22f 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-1.c > > > @@ -51,7 +51,7 @@ struct socket { > > > > > > struct sco_conninfo { > > > __u16 hci_handle; > > > - __u8 dev_class[3]; /* { dg-message "padding after field > > > 'dev_class' is uninitialized \\(1 byte\\)" } */ > > > + __u8 dev_class[3]; /* { dg-message "padding after field > > > 'dev_class' is uninitialized \\(1 byte\\)" "padding" { target { ! > > > default_packed } } } */ > > > }; > > > > > > struct sco_conn { > > > @@ -83,8 +83,8 @@ static int > > > sco_sock_getsockopt_old_broken(struct > > > socket *sock, int optname, char > > > { > > > struct sock *sk = sock->sk; > > > /* [...snip...] */ > > > - struct sco_conninfo cinfo; /* { dg-message "region > > > created on > > > stack here" "where" } */ > > > - /* { dg-message "capacity: 6 > > > bytes" "capacity" { target *-*-* } .-1 } */ > > > + struct sco_conninfo cinfo; /* { dg-message "region > > > created on > > > stack here" "where" { target { ! default_packed } } } */ > > > + /* { dg-message "capacity: 6 > > > bytes" "capacity" { target { ! default_packed } } .-1 } */ > > > /* Note: 40 bits of fields, padded to 48. */ > > > > > > int len, err = 0; > > > @@ -101,8 +101,8 @@ static int > > > sco_sock_getsockopt_old_broken(struct > > > socket *sock, int optname, char > > > memcpy(cinfo.dev_class, sco_pi(sk)->conn->hcon- > > > > dev_class, 3); > > > > > > len = min_t(unsigned int, len, sizeof(cinfo)); > > > - if (copy_to_user(optval, (char *)&cinfo, len)) /* > > > { > > > dg-warning "potential exposure of sensitive information by > > > copying > > > uninitialized data from stack" "warning" { target *-*-* } } */ > > > - /* { dg-message "1 byte is uninitialized" > > > "how much note" { target *-*-* } .-1 } */ > > > + if (copy_to_user(optval, (char *)&cinfo, len)) /* > > > { > > > dg-warning "potential exposure of sensitive information by > > > copying > > > uninitialized data from stack" "warning" { target { ! > > > default_packed > > > } } } */ > > > + /* { dg-message "1 byte is uninitialized" > > > "how much note" { target { ! default_packed } } .-1 } */ > > > err = -1; > > > > > > /* [...snip...] */ > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078- > > > 2.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c > > > index 2096bda71798..d5f598b0878e 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2011-1078-2.c > > > @@ -15,22 +15,22 @@ typedef unsigned short __u16; > > > > > > struct sco_conninfo { > > > __u16 hci_handle; > > > - __u8 dev_class[3]; /* { dg-message "padding after field > > > 'dev_class' is uninitialized \\(1 byte\\)" } */ > > > + __u8 dev_class[3]; /* { dg-message "padding after field > > > 'dev_class' is uninitialized \\(1 byte\\)" "padding" { target { > > > ! > > > default_packed } } } */ > > > }; > > > > > > /* Adapted from sco_sock_getsockopt_old in net/bluetooth/sco.c. > > > */ > > > > > > int test_1 (char __user *optval, const struct sco_conninfo *in) > > > { > > > - struct sco_conninfo cinfo; /* { dg-message "region > > > created on > > > stack here" "where" } */ > > > - /* { dg-message "capacity: 6 > > > bytes" "capacity" { target *-*-* } .-1 } */ > > > + struct sco_conninfo cinfo; /* { dg-message "region > > > created on > > > stack here" "where" { target { ! default_packed } } } */ > > > + /* { dg-message "capacity: 6 > > > bytes" "capacity" { target { ! default_packed } } .-1 } */ > > > /* Note: 40 bits of fields, padded to 48. */ > > > > > > cinfo.hci_handle = in->hci_handle; > > > memcpy(cinfo.dev_class, in->dev_class, 3); > > > > > > - copy_to_user(optval, &cinfo, sizeof(cinfo)); /* { dg- > > > warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" "warning" } */ > > > - /* { dg-message "1 byte is uninitialized" "how much note" > > > { > > > target *-*-* } .-1 } */ > > > + copy_to_user(optval, &cinfo, sizeof(cinfo)); /* { dg- > > > warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" "warning" { target { ! default_packed } } } */ > > > + /* { dg-message "1 byte is uninitialized" "how much note" > > > { > > > target { ! default_packed } } .-1 } */ > > > } > > > > > > int test_2 (char __user *optval, const struct sco_conninfo *in) > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549- > > > 1.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c > > > index 8a1c816cc1b5..8fcf9a904a2a 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18549-1.c > > > @@ -35,7 +35,7 @@ struct aac_srb_reply > > > __le32 scsi_status; > > > __le32 data_xfer_length; > > > __le32 sense_data_size; > > > - u8 sense_data[AAC_SENSE_BUFFERSIZE]; /* { > > > dg- > > > message "padding after field 'sense_data' is uninitialized \\(2 > > > bytes\\)" } */ > > > + u8 sense_data[AAC_SENSE_BUFFERSIZE]; /* { > > > dg- > > > message "padding after field 'sense_data' is uninitialized \\(2 > > > bytes\\)" "padding" { target { ! default_packed } } } */ > > > }; > > > > > > #define ST_OK 0 > > > @@ -50,8 +50,8 @@ static int aac_send_raw_srb(/* [...snip...] */ > > > > > > /* [...snip...] */ > > > > > > - struct aac_srb_reply reply; /* { dg-message "region > > > created > > > on stack here" "memspace message" } */ > > > - /* { dg-message "capacity: 52 bytes" "capacity message" { > > > target *-*-* } .-1 } */ > > > + struct aac_srb_reply reply; /* { dg-message "region > > > created > > > on stack here" "memspace message" { target { ! default_packed } } > > > } > > > */ > > > + /* { dg-message "capacity: 52 bytes" "capacity message" { > > > target { ! default_packed } } .-1 } */ > > > > > > reply.status = ST_OK; > > > > > > @@ -65,8 +65,8 @@ static int aac_send_raw_srb(/* [...snip...] */ > > > > > > /* [...snip...] */ > > > > > > - if (copy_to_user(user_reply, &reply, /* { dg-warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" } */ > > > - /* { dg-message "2 > > > bytes > > > are uninitialized" "note how much" { target *-*-* } .-1 } */ > > > + if (copy_to_user(user_reply, &reply, /* { dg-warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" "padding" { target { ! default_packed } } } */ > > > + /* { dg-message "2 > > > bytes > > > are uninitialized" "note how much" { target { ! default_packed } > > > } .- > > > 1 } */ > > > sizeof(struct aac_srb_reply))) { > > > /* [...snip...] */ > > > } > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550- > > > 1.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c > > > index 4272da96bab0..a7361f937401 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-CVE-2017-18550-1.c > > > @@ -31,7 +31,7 @@ struct aac_hba_info { > > > u8 driver_name[50]; /* { dg-message "field > > > 'driver_name' > > > is uninitialized \\(50 bytes\\)" } */ > > > u8 adapter_number; > > > u8 system_io_bus_number; > > > - u8 device_number; /* { dg-message "padding after > > > field > > > 'device_number' is uninitialized \\(3 bytes\\)" } */ > > > + u8 device_number; /* { dg-message "padding after > > > field > > > 'device_number' is uninitialized \\(3 bytes\\)" "padding" { > > > target { > > > ! default_packed } } } */ > > > u32 function_number; > > > u32 vendor_id; > > > u32 device_id; > > > @@ -108,7 +108,8 @@ struct pci_bus { > > > static int aac_get_hba_info(struct aac_dev *dev, void __user > > > *arg) > > > { > > > struct aac_hba_info hbainfo; /* { dg-message "region > > > created > > > on stack here" "memspace message" } */ > > > - /* { dg-message "capacity: 200 bytes" "capacity message" > > > { > > > target *-*-* } .-1 } */ > > > + /* { dg-message "capacity: 200 bytes" "capacity message" > > > { > > > target { ! default_packed } } .-1 } */ > > > + /* { dg-message "capacity: 194 bytes" "capacity message" > > > { > > > target default_packed } .-2 } */ > > > > > > hbainfo.adapter_number = (u8) dev->id; > > > hbainfo.system_io_bus_number = dev->pdev->bus->number; > > > @@ -121,7 +122,7 @@ static int aac_get_hba_info(struct aac_dev > > > *dev, > > > void __user *arg) > > > hbainfo.sub_system_id = dev->pdev- > > > > subsystem_device; > > > > > > if (copy_to_user(arg, &hbainfo, sizeof(struct > > > aac_hba_info))) > > > { /* { dg-warning "potential exposure of sensitive information by > > > copying uninitialized data from stack" "warning" } */ > > > - /* { dg-message "177 bytes are uninitialized" > > > "how > > > much" { target *-*-* } .-1 } */ > > > + /* { dg-message "177 bytes are uninitialized" > > > "how > > > much" { target { ! default_packed } } .-1 } */ > > > /* [...snip...] */ > > > } > > > > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns- > > > 1.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c > > > index 500845364388..d9db2b3bdebc 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-antipatterns-1.c > > > @@ -90,21 +90,21 @@ int infoleak_heap_missing_a_field(void __user > > > *dst, u32 v) > > > > > > struct infoleak_3 > > > { > > > - u8 a; /* { dg-message "padding after field 'a' is > > > uninitialized > > > \\(3 bytes\\)" } */ > > > + u8 a; /* { dg-message "padding after field 'a' is > > > uninitialized > > > \\(3 bytes\\)" "padding" { target { ! default_packed } } } */ > > > /* padding here */ > > > u32 b; > > > }; > > > > > > int infoleak_stack_padding(void __user *dst, u8 p, u32 q) > > > { > > > - struct infoleak_3 st; /* { dg-message "region created on stack > > > here" "where" } */ > > > - /* { dg-message "capacity: 8 bytes" "capacity" { target *-*-* > > > } .- > > > 1 } */ > > > + struct infoleak_3 st; /* { dg-message "region created on stack > > > here" "where" { target { ! default_packed } } } */ > > > + /* { dg-message "capacity: 8 bytes" "capacity" { target { ! > > > default_packed } } .-1 } */ > > > > > > st.a = p; > > > st.b = q; > > > /* No initialization of padding. */ > > > - if (copy_to_user(dst, &st, sizeof(st))) /* { dg-warning > > > "potential > > > exposure of sensitive information by copying uninitialized data > > > from > > > stack" "warning" } */ > > > - /* { dg-message "3 bytes are uninitialized" "note how much" > > > { > > > target *-*-* } .-1 } */ > > > + if (copy_to_user(dst, &st, sizeof(st))) /* { dg-warning > > > "potential > > > exposure of sensitive information by copying uninitialized data > > > from > > > stack" "warning" { target { ! default_packed } } } */ > > > + /* { dg-message "3 bytes are uninitialized" "note how much" > > > { > > > target { ! default_packed } } .-1 } */ > > > return -EFAULT; > > > return 0; > > > } > > > diff --git a/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c > > > b/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c > > > index 6961b44f76b9..192c9e7802ad 100644 > > > --- a/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c > > > +++ b/gcc/testsuite/gcc.dg/plugin/infoleak-fixit-1.c > > > @@ -11,16 +11,18 @@ typedef unsigned int u32; > > > > > > struct st > > > { > > > - u8 i; /* { dg-message "padding after field 'i' is > > > uninitialized > > > \\(3 bytes\\)" } */ > > > + u8 i; /* { dg-message "padding after field 'i' is > > > uninitialized > > > \\(3 bytes\\)" "padding" { target { ! default_packed } } } */ > > > u32 j; /* { dg-message "field 'j' is uninitialized \\(4 > > > bytes\\)" > > > } */ > > > }; > > > > > > void test (void __user *dst, u8 a) > > > { > > > struct st s; /* { dg-message "region created on stack here" > > > "where" } */ > > > - /* { dg-message "capacity: 8 bytes" "capacity" { target *-*-* > > > } .- > > > 1 } */ > > > - /* { dg-message "suggest forcing zero-initialization by > > > providing > > > a '.0.' initializer" "fix-it hint" { target *-*-* } .-2 } */ > > > + /* { dg-message "capacity: 8 bytes" "capacity" { target { ! > > > default_packed } } .-1 } */ > > > + /* { dg-message "capacity: 5 bytes" "capacity" { target > > > default_packed } .-2 } */ > > > + /* { dg-message "suggest forcing zero-initialization by > > > providing > > > a '.0.' initializer" "fix-it hint" { target *-*-* } .-3 } */ > > > s.i = a; > > > copy_to_user(dst, &s, sizeof (struct st)); /* { dg-warning > > > "potential exposure of sensitive information by copying > > > uninitialized > > > data from stack" "warning" } */ > > > - /* { dg-message "7 bytes are uninitialized" "note how much" { > > > target *-*-* } .-1 } */ > > > + /* { dg-message "7 bytes are uninitialized" "note how much" { > > > target { ! default_packed } } .-1 } */ > > > + /* { dg-message "4 bytes are uninitialized" "note how much" { > > > target default_packed } .-2 } */ > > > } > > >