| Message ID | 20230323160508.672397-2-teo.coupriediaz@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | syscalls/sockioctl: Align buffer to struct ifreq | expand |
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; }
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; > } > >
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
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 > >
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);
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(-)