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

[Teo Couprie Diaz]

Hi Li,

On 27/03/2023 09:35, Li Wang wrote:
> 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.
Thanks for clarifying Li, then I will simply send out the v2 with 
Cyril's changes
and updated commit message.

Best regards,
Téo

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