From: Hans-Peter Nilsson <hp@axis.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] testsuite: Handle default_packed targets in gcc.dg/plugin
Date: Thu, 16 Mar 2023 19:25:05 +0100 [thread overview]
Message-ID: <20230316182505.7EBB1203D2@pchp3.se.axis.com> (raw)
In-Reply-To: <ac991d7a7c26980508490ed3cb15571f855989a5.camel@redhat.com> (message from David Malcolm on Thu, 16 Mar 2023 13:55:48 -0400)
> From: David Malcolm <dmalcolm@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?
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 } */
> > }
>
next prev parent reply other threads:[~2023-03-16 18:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-09 18:56 Hans-Peter Nilsson
2023-03-16 15:29 ` Ping: " Hans-Peter Nilsson
2023-03-16 17:55 ` David Malcolm
2023-03-16 18:25 ` Hans-Peter Nilsson [this message]
2023-03-16 18:42 ` David Malcolm
2023-03-17 16:10 ` Hans-Peter Nilsson
2023-03-17 16:43 ` David Malcolm
2023-03-16 18:45 ` Hans-Peter Nilsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230316182505.7EBB1203D2@pchp3.se.axis.com \
--to=hp@axis.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).