On 15/03/2023 16:29, Teo Couprie Diaz wrote:

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.

This is exacerbated in purecap because of the increased alignment constraint of struct ifreq.

Force the alignment of buf to that of struct ifreq.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

The line mentionning purecap will be dropped when upstreaming. v2:

• Use struct ifreq directly for alignment
• Reformulate comments and commit message as the undefined behavior is not spectific to purecap
• Change the attribute definition to be closer to more similar to the rest of the codebase. Should be better for upstream.

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

LGTM. FWIW this buffer should really be aligned in all cases, because it is always passed to the SIOCGIFCONF ioctl, which is what is writing those struct ifreq's there in the first place. I suspect this will never fail in practice because copy_to_user() will make sure not to perform any unaligned store if that would fault, but conceptually it makes sense to have at least the alignment of struct ifreq.

Kevin