[LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq

[Li Wang]

Hi Teo,

On Fri, Mar 24, 2023 at 10:35 PM Teo Couprie Diaz
<teo.coupriediaz@arm.com> wrote:
>
> Hi Cyril,
>
> On 24/03/2023 11:52, Cyril Hrubis wrote:
> > Hi!
> >> In setup3, the following line can lead to an undefined behavior:
> >>    ifr = *(struct ifreq *)ifc.ifc_buf;
> >>
> >> Indeed, at this point it can be assumed that ifc.ifc_buf is suitably
> >> aligned for struct ifreq.
> >> However, ifc.ifc_buf is assigned to buf which has no alignment
> >> constraints. This means there exists cases where buf is not suitably
> >> aligned to load a struct ifreq, which can generate a SIGBUS.
> >>
> >> Force the alignment of buf to that of struct ifreq.
> >>
> >> Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
> >> ---
> >> CI Build: https://github.com/Teo-CD/ltp/actions/runs/4502204132
> >>
> >>   testcases/kernel/syscalls/sockioctl/sockioctl01.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> >> index 486236af9d6b..e63aa1921877 100644
> >> --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> >> +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> >> @@ -52,7 +52,13 @@ static struct ifreq ifr;
> >>   static int sinlen;
> >>   static int optval;
> >>
> >> -static char buf[8192];
> >> +/*
> >> + * buf has no alignment constraints by default. However, it is used to load
> >> + * a struct ifreq in setup3, which requires it to have an appropriate alignment
> >> + * to prevent a possible undefined behavior.
> >> + */
> >> +static char buf[8192]
> >> +    __attribute__((aligned(__alignof__(struct ifreq))));
> >>
> >>   static void setup(void);
> >>   static void setup0(void);
> > Looking at the code, shouldn't we rather than that declare the buffer as
> > an struct ifreq array, that would naturally align the buffer without any
> > need for tricky __attribute__.
> __attribute__+__alignof__ is quite unwieldy indeed. I kept the char* to
> match the struct definition,
> but it's really just to represent a pointer to something. It's not used
> as anything else in the test anyway.
>
> If you feel there's no good reason to keep the char* buff and cast to
> void* for the syscall,
> I agree that it would be better. I tested on our system which generated
> the fault initially
> and it works fine as expected.
>
> What would be the procedure in this case ? Shall I resend the patch with
> your changes ?

Yes, you need to send a patch v2 with Cyril's suggestion.

> Would you just apply it or send it yourself ? Happy to follow up however
> is best.
>
> Thanks for taking the time to look into it,
> Best regards
> Téo
> >
> > diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> > index 51dac9c16..206a4999e 100644
> > --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> > +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c
> > @@ -52,7 +52,7 @@ static struct ifreq ifr;
> >   static int sinlen;
> >   static int optval;
> >
> > -static char buf[8192];
> > +static struct ifreq buf[200];
> >
> >   static void setup(void);
> >   static void setup0(void);
> > @@ -218,7 +218,7 @@ static void setup2(void)
> >          s = SAFE_SOCKET(cleanup, tdat[testno].domain, tdat[testno].type,
> >                          tdat[testno].proto);
> >          ifc.ifc_len = sizeof(buf);
> > -       ifc.ifc_buf = buf;
> > +       ifc.ifc_buf = (void*)buf;
> >   }
> >
> >
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp



-- 
Regards,
Li Wang