In setup3, the following line introduces an undefined behavior in purecap: ifr = *(struct ifreq *)ifc.ifc_buf;
Indeed, at this point it can be assumed that ifc.ifc_buf is suitably aligned for struct ifreq, which is 16 in purecap as it stores a capability. 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 will produce a segmentation fault.
Force the alignment of buf to 16 bytes in purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- testcases/kernel/syscalls/sockioctl/sockioctl01.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c index 486236af9d6b..22e96b42ca0c 100644 --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c @@ -42,6 +42,18 @@ #include "test.h" #include "safe_macros.h"
+/* + * Aligning to 16 bytes is required in purecap as the test loads a struct ifreq + * from buf, which is 16 bytes aligned and might be loaded via capability + * registers. Unsuitable alignment would be an undefined behavior and could lead + * to a segmentation fault. + */ +#ifdef __CHERI_PURE_CAPABILITY__ +#define BUF_ALIGN __attribute__((aligned(16))) +#else +#define BUF_ALIGN +#endif + char *TCID = "sockioctl01"; int testno;
@@ -52,7 +64,7 @@ static struct ifreq ifr; static int sinlen; static int optval;
-static char buf[8192]; +static BUF_ALIGN char buf[8192];
static void setup(void); static void setup0(void);
On 14-03-2023 14:53, Teo Couprie Diaz wrote:
In setup3, the following line introduces an undefined behavior in purecap: ifr = *(struct ifreq *)ifc.ifc_buf;
Indeed, at this point it can be assumed that ifc.ifc_buf is suitably aligned for struct ifreq, which is 16 in purecap as it stores a capability. 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 will produce a segmentation fault.
I think you mean a bus error instead of a segmentation fault.
Force the alignment of buf to 16 bytes in purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/sockioctl/sockioctl01.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c index 486236af9d6b..22e96b42ca0c 100644 --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c @@ -42,6 +42,18 @@ #include "test.h" #include "safe_macros.h" +/*
- Aligning to 16 bytes is required in purecap as the test loads a struct ifreq
- from buf, which is 16 bytes aligned and might be loaded via capability
- registers. Unsuitable alignment would be an undefined behavior and could lead
- to a segmentation fault.
- */
+#ifdef __CHERI_PURE_CAPABILITY__ +#define BUF_ALIGN __attribute__((aligned(16))) +#else +#define BUF_ALIGN +#endif
I don't think this is a Purecap issue really. I think the Purecap case just surfaced the undefined behaviour, but the fix should be generic. The C standard mentions that "A pointer to an object type may be converted to a pointer to a different object type. If the resulting pointer is not correctly aligned for the referenced type, the behavior is undefined." So IIUC, the UB is there regardless of the arch. I think a generic "__attribute__((aligned(__alignof__(struct ifreq)))) would work, but maybe I'm missing something.
Thanks, Tudor
char *TCID = "sockioctl01"; int testno; @@ -52,7 +64,7 @@ static struct ifreq ifr; static int sinlen; static int optval; -static char buf[8192]; +static BUF_ALIGN char buf[8192]; static void setup(void); static void setup0(void);
On 14/03/2023 15:17, Tudor Cretu wrote:
On 14-03-2023 14:53, Teo Couprie Diaz wrote:
In setup3, the following line introduces an undefined behavior in purecap: ifr = *(struct ifreq *)ifc.ifc_buf;
Indeed, at this point it can be assumed that ifc.ifc_buf is suitably aligned for struct ifreq, which is 16 in purecap as it stores a capability. 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 will produce a segmentation fault.
I think you mean a bus error instead of a segmentation fault.
I wasn't sure which one to put to be honest. In user space I believe it did show up as a segmentation fault, but I might be misremembering and the bus fault would probably be more correct.
Force the alignment of buf to 16 bytes in purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/sockioctl/sockioctl01.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c index 486236af9d6b..22e96b42ca0c 100644 --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c @@ -42,6 +42,18 @@ #include "test.h" #include "safe_macros.h" +/*
- Aligning to 16 bytes is required in purecap as the test loads a
struct ifreq
- from buf, which is 16 bytes aligned and might be loaded via
capability
- registers. Unsuitable alignment would be an undefined behavior
and could lead
- to a segmentation fault.
- */
+#ifdef __CHERI_PURE_CAPABILITY__ +#define BUF_ALIGN __attribute__((aligned(16))) +#else +#define BUF_ALIGN +#endif
I don't think this is a Purecap issue really. I think the Purecap case just surfaced the undefined behaviour, but the fix should be generic. The C standard mentions that "A pointer to an object type may be converted to a pointer to a different object type. If the resulting pointer is not correctly aligned for the referenced type, the behavior is undefined." So IIUC, the UB is there regardless of the arch. I think a generic "__attribute__((aligned(__alignof__(struct ifreq)))) would work, but maybe I'm missing something.
Mh, no I believe you are right. I did try to do that but didn't find `__alignof__`, only the C11 keyword `alignof` which obviously didn't work because we build for an earlier standard version.
That would indeed be a better fix and would be needed regardless of purecap. (Even though I guess the compiler always aligned it to 8-bytes, which is the reason it worked in regular Aarch64 and I imagine x86 as well.)
I will change to that and update the wordings accordingly.
Thanks, Tudor
Thanks for the review,
Best regards Téo
char *TCID = "sockioctl01"; int testno; @@ -52,7 +64,7 @@ static struct ifreq ifr; static int sinlen; static int optval; -static char buf[8192]; +static BUF_ALIGN char buf[8192]; static void setup(void); static void setup0(void);
On 14/03/2023 16:33, Teo Couprie Diaz wrote:
On 14/03/2023 15:17, Tudor Cretu wrote:
On 14-03-2023 14:53, Teo Couprie Diaz wrote:
In setup3, the following line introduces an undefined behavior in purecap: ifr = *(struct ifreq *)ifc.ifc_buf;
Indeed, at this point it can be assumed that ifc.ifc_buf is suitably aligned for struct ifreq, which is 16 in purecap as it stores a capability. 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 will produce a segmentation fault.
I think you mean a bus error instead of a segmentation fault.
I wasn't sure which one to put to be honest. In user space I believe it did show up as a segmentation fault, but I might be misremembering and the bus fault would probably be more correct.
I'm pretty sure you'll get a SIGBUS too, that's the standard signal for alignment faults.
Force the alignment of buf to 16 bytes in purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/sockioctl/sockioctl01.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/testcases/kernel/syscalls/sockioctl/sockioctl01.c b/testcases/kernel/syscalls/sockioctl/sockioctl01.c index 486236af9d6b..22e96b42ca0c 100644 --- a/testcases/kernel/syscalls/sockioctl/sockioctl01.c +++ b/testcases/kernel/syscalls/sockioctl/sockioctl01.c @@ -42,6 +42,18 @@ #include "test.h" #include "safe_macros.h" +/*
- Aligning to 16 bytes is required in purecap as the test loads a
struct ifreq
- from buf, which is 16 bytes aligned and might be loaded via
capability
- registers. Unsuitable alignment would be an undefined behavior
and could lead
- to a segmentation fault.
- */
+#ifdef __CHERI_PURE_CAPABILITY__ +#define BUF_ALIGN __attribute__((aligned(16))) +#else +#define BUF_ALIGN +#endif
I don't think this is a Purecap issue really. I think the Purecap case just surfaced the undefined behaviour, but the fix should be generic. The C standard mentions that "A pointer to an object type may be converted to a pointer to a different object type. If the resulting pointer is not correctly aligned for the referenced type, the behavior is undefined." So IIUC, the UB is there regardless of the arch. I think a generic "__attribute__((aligned(__alignof__(struct ifreq)))) would work, but maybe I'm missing something.
Mh, no I believe you are right. I did try to do that but didn't find `__alignof__`, only the C11 keyword `alignof` which obviously didn't work because we build for an earlier standard version.
That would indeed be a better fix and would be needed regardless of purecap. (Even though I guess the compiler always aligned it to 8-bytes, which is the reason it worked in regular Aarch64 and I imagine x86 as well.)
I will change to that and update the wordings accordingly.
I think this is best indeed. And as Tudor said, this is a generic issue that just happened to be revealed in purecap, so this is a very good candidate for upstreaming - the UB in the original code is indisputable.
Kevin
Thanks, Tudor
Thanks for the review,
Best regards Téo
char *TCID = "sockioctl01"; int testno; @@ -52,7 +64,7 @@ static struct ifreq ifr; static int sinlen; static int optval; -static char buf[8192]; +static BUF_ALIGN char buf[8192]; static void setup(void); static void setup0(void);
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp@op-lists.linaro.org
-
Kevin Brodsky -
Teo Couprie Diaz -
Tudor Cretu