Re: [PATCH for-6.2 0/7] linux-user: Clean up siginfo_t handling for arm, aarch64

[Laurent Vivier]

Le 13/08/2021 à 15:18, Peter Maydell a écrit :
> Coverity reported that we don't set the _sifields union when queuing
> the SIGTRAP for EXCP_DEBUG events on aarch64. This series fixes that
> bug and a few others, and cleans up the way we queue fault signals
> to be less error-prone.
> 
> The underlying cause of the bug is that when queueing a signal
> the code must set the right field in the _sifields union depending
> on the si_type that it passes to queue_signal(), and there's nothing
> guarding against forgetting to do this. The "fill in a whole struct
> and then call queue_signal()" code is also a bit longwinded. In the
> real kernel, there is a function force_sig_fault() which does this
> for the SI_FAULT signal type. The series provides a QEMU implementation
> of this (which goes alongside the existing force_sig() that does this
> for SI_KILL signals), and uses it in the arm and aarch64 code.
> Because force_sig_fault() takes the address as an argument, it's
> not possible for the caller to forget to fill it in.
> 
> Obviously we should also convert the other targets where they are
> directly calling queue_signal() (there are other places that forget
> to fill in the sifields union fields; I don't know why Coverity
> hasn't spotted those). But I thought it better to send this out
> for review of the new API and approach before spending time on
> converting other targets. (In particular fixing places where
> EXCP_DEBUG handling forgets to set the sifields address is a
> pain, because in the real kernel different architectures might
> either report the PC or else report a zero address here, so it
> requires looking into the kernel sources to check which...)
> Once all the archs are cleaned up we might be able to make
> queue_signal() static so it's local to signal.c.
> 
> PS: there's probably a trivial conflict with my include-file-split
> patchseries.
> 
> thanks
> -- PMM
> 
> Peter Maydell (7):
>   linux-user/aarch64: Set siginfo_t addr field for SIGTRAP signals
>   linux-user/arm: Set siginfo_t addr field for SIGTRAP signals
>   linux-user/arm: Use force_sig() to deliver fpa11 emulation SIGFPE
>   linux-user: Zero out target_siginfo_t in force_sig()
>   linux-user: Provide new force_sig_fault() function
>   linux-user/arm: Use force_sig_fault()
>   linux-user/aarch64: Use force_sig_fault()
> 
>  linux-user/signal-common.h    |  1 +
>  linux-user/aarch64/cpu_loop.c | 33 +++++-------------
>  linux-user/arm/cpu_loop.c     | 64 +++++++++++------------------------
>  linux-user/signal.c           | 19 ++++++++++-
>  4 files changed, 48 insertions(+), 69 deletions(-)
> 

Applied to my linux-user-for-6.2 branch.

Thanks,
Laurent