mbox series

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

Message ID 20210813131809.28655-1-peter.maydell@linaro.org
Headers show
Series linux-user: Clean up siginfo_t handling for arm, aarch64 | expand

Message

Peter Maydell Aug. 13, 2021, 1:18 p.m. UTC 
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(-)

-- 
2.20.1

Comments

Laurent Vivier Sept. 23, 2021, 1:12 p.m. UTC | #1 
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