| Message ID | 20230625204907.57291-1-julien@xen.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | xen/arm: Enable UBSAN support | expand |
On 25.06.2023 22:48, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Hi all, > > At the moment, we are not able to enable UBSAN on Arm because the > final binary will be over the maximum size of Xen we currently support > (i.e. 2MB). > > This patch series aim to lift the restrictions and also > enable UBSAN. Lastly, at the end of the series, there is the first > issue found by USBAN. > > There are a few of others. One will be fixed by the MISRA work > in [1] and the other is a bit tricky. One the splat is (the > others seems to be for similar reasons) > > (XEN) ================================================================================ > (XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5 > (XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data' > (XEN) which requires 8 byte alignment > (XEN) Xen WARN at common/ubsan/ubsan.c:172 > > This is on 32-bit and UBSAN seems to complain about the check in > list_for_each_entry. I haven't yet dived into the issue yet. At a guess this is because the list head struct, living in struct csched2_private, has only 32-bit alignment, while on the last loop iteration pos doesn't hold a real struct csched2_runqueue_data * (which ought to b 64-bit aligned, but isn't in the special case of having reached the list head again). If I'm not mistaken rwlock_t is 12 bytes for Arm32, which would match with the address above ending in c, assuming that xmalloc() returns at least 16-byte-aligned space on Arm32 as well. If so, in this particular case some rearrangement of struct csched2_private ought to help, but as you say this is a more general issue and hence likely wants addressing in a generic way. Jan
(+ The rest) Hi Jan, On 26/06/2023 06:45, Jan Beulich wrote: > On 25.06.2023 22:48, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Hi all, >> >> At the moment, we are not able to enable UBSAN on Arm because the >> final binary will be over the maximum size of Xen we currently support >> (i.e. 2MB). >> >> This patch series aim to lift the restrictions and also >> enable UBSAN. Lastly, at the end of the series, there is the first >> issue found by USBAN. >> >> There are a few of others. One will be fixed by the MISRA work >> in [1] and the other is a bit tricky. One the splat is (the >> others seems to be for similar reasons) >> >> (XEN) ================================================================================ >> (XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5 >> (XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data' >> (XEN) which requires 8 byte alignment >> (XEN) Xen WARN at common/ubsan/ubsan.c:172 >> >> This is on 32-bit and UBSAN seems to complain about the check in >> list_for_each_entry. I haven't yet dived into the issue yet. > > At a guess this is because the list head struct, living in > struct csched2_private, has only 32-bit alignment, while on the > last loop iteration pos doesn't hold a real struct > csched2_runqueue_data * (which ought to b 64-bit aligned, but > isn't in the special case of having reached the list head again). > If I'm not mistaken rwlock_t is 12 bytes for Arm32, which would > match with the address above ending in c, assuming that xmalloc() > returns at least 16-byte-aligned space on Arm32 as well. If so, > in this particular case some rearrangement of struct > csched2_private ought to help, but as you say this is a more > general issue and hence likely wants addressing in a generic way. Your analysis is correct. So far I have only seen the splats in the credit2 code. But it feels a little be odd to fix only there. Our list implementation is based on the Linux one. I checked a recent version but I couldn't find any hints that they may have fixed the issue. For a more generic fix, I was thinking to force the alignment of the list head to 8-bytes. This is not perfect but would cover most of the use-cases where list head is used. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: [PATCH 0/9] xen/arm: Enable UBSAN support > > Julien Grall (9): > xen/arm: Check Xen size when linking > xen/arm64: head: Don't map too much in boot_third > xen/arm32: head: Don't map too much in boot_third > xen/arm32: head: Remove 'r6' from the clobber list of > create_page_tables() > xen/arm: Rework the code mapping Xen to avoid relying on the size of > Xen > xen/arm64: entry: Don't jump outside of an alternative > xen/arm64: head: Rework PRINT() to work when the string is not withing > +/- 1MB > xen/arm: Allow the user to build Xen with USBAN > xen/arm32: vfp: Add missing U for shifted constant I put the series in our CI this morning and I can now confirm each of these patches will not breaking existing current Xen functionality, so: Tested-by: Henry Wang <Henry.Wang@arm.com> I put the tag in the cover letter to avoid the spamming mails in the mailing list. Feel free to apply the tag to each commit if you want to. Kind regards, Henry
From: Julien Grall <jgrall@amazon.com> Hi all, At the moment, we are not able to enable UBSAN on Arm because the final binary will be over the maximum size of Xen we currently support (i.e. 2MB). This patch series aim to lift the restrictions and also enable UBSAN. Lastly, at the end of the series, there is the first issue found by USBAN. There are a few of others. One will be fixed by the MISRA work in [1] and the other is a bit tricky. One the splat is (the others seems to be for similar reasons) (XEN) ================================================================================ (XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5 (XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data' (XEN) which requires 8 byte alignment (XEN) Xen WARN at common/ubsan/ubsan.c:172 This is on 32-bit and UBSAN seems to complain about the check in list_for_each_entry. I haven't yet dived into the issue yet. Cheers, [1] cover.1687250177.git.gianluca.luparini@bugseng.com Julien Grall (9): xen/arm: Check Xen size when linking xen/arm64: head: Don't map too much in boot_third xen/arm32: head: Don't map too much in boot_third xen/arm32: head: Remove 'r6' from the clobber list of create_page_tables() xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen xen/arm64: entry: Don't jump outside of an alternative xen/arm64: head: Rework PRINT() to work when the string is not withing +/- 1MB xen/arm: Allow the user to build Xen with USBAN xen/arm32: vfp: Add missing U for shifted constant xen/arch/arm/Kconfig | 1 + xen/arch/arm/arm32/head.S | 79 +++++++++++++++++++++------- xen/arch/arm/arm64/entry.S | 21 ++++++-- xen/arch/arm/arm64/head.S | 68 +++++++++++++++++++----- xen/arch/arm/include/asm/arm32/vfp.h | 18 +++---- xen/arch/arm/include/asm/config.h | 19 +++---- xen/arch/arm/include/asm/lpae.h | 8 +-- xen/arch/arm/mm.c | 24 +++++---- xen/arch/arm/xen.lds.S | 4 ++ 9 files changed, 176 insertions(+), 66 deletions(-)