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

[Teo Couprie Diaz <teo.coupriediaz@arm.com>]

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 ?
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