* [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq
@ 2023-03-23 16:05 Teo Couprie Diaz
2023-03-24 11:52 ` Cyril Hrubis
0 siblings, 1 reply; 5+ messages in thread
From: Teo Couprie Diaz @ 2023-03-23 16:05 UTC (permalink / raw)
To: ltp
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);
--
2.34.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq
2023-03-23 16:05 [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq Teo Couprie Diaz
@ 2023-03-24 11:52 ` Cyril Hrubis
2023-03-24 14:34 ` Teo Couprie Diaz
0 siblings, 1 reply; 5+ messages in thread
From: Cyril Hrubis @ 2023-03-24 11:52 UTC (permalink / raw)
To: Teo Couprie Diaz; +Cc: ltp
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__.
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;
}
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq
2023-03-24 11:52 ` Cyril Hrubis
@ 2023-03-24 14:34 ` Teo Couprie Diaz
2023-03-27 8:35 ` Li Wang
0 siblings, 1 reply; 5+ messages in thread
From: Teo Couprie Diaz @ 2023-03-24 14:34 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: ltp
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq
2023-03-24 14:34 ` Teo Couprie Diaz
@ 2023-03-27 8:35 ` Li Wang
2023-03-27 10:25 ` Teo Couprie Diaz
0 siblings, 1 reply; 5+ messages in thread
From: Li Wang @ 2023-03-27 8:35 UTC (permalink / raw)
To: Teo Couprie Diaz; +Cc: ltp
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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq
2023-03-27 8:35 ` Li Wang
@ 2023-03-27 10:25 ` Teo Couprie Diaz
0 siblings, 0 replies; 5+ messages in thread
From: Teo Couprie Diaz @ 2023-03-27 10:25 UTC (permalink / raw)
To: Li Wang; +Cc: ltp
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
>
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-27 10:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-23 16:05 [LTP] [PATCH] syscalls/sockioctl: Align buffer to struct ifreq Teo Couprie Diaz
2023-03-24 11:52 ` Cyril Hrubis
2023-03-24 14:34 ` Teo Couprie Diaz
2023-03-27 8:35 ` Li Wang
2023-03-27 10:25 ` Teo Couprie Diaz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.