Re: [PATCH 8/8] signal: Remove kernel interal si_code magic

[Andrei Vagin]

On Fri, Jun 30, 2017 at 07:39:06AM -0500, Eric W. Biederman wrote:
> struct siginfo is a union and the kernel since 2.4 has been hiding a union
> tag in the high 16bits of si_code using the values:
> __SI_KILL
> __SI_TIMER
> __SI_POLL
> __SI_FAULT
> __SI_CHLD
> __SI_RT
> __SI_MESGQ
> __SI_SYS
> 
> While this looks plausible on the surface, in practice this situation has
> not worked well.
> 
> - Injected positive signals are not copied to user space properly
>   unless they have these magic high bits set.
> 
> - Injected positive signals are not reported properly by signalfd
>   unless they have these magic high bits set.
> 
> - These kernel internal values leaked to userspace via ptrace_peek_siginfo
> 
> - It was possible to inject these kernel internal values and cause the
>   the kernel to misbehave.
> 
> - Kernel developers got confused and expected these kernel internal values
>   in userspace in kernel self tests.
> 
> - Kernel developers got confused and set si_code to __SI_FAULT which
>   is SI_USER in userspace which causes userspace to think an ordinary user
>   sent the signal and that it was not kernel generated.
> 
> - The values make it impossible to reorganize the code to transform
>   siginfo_copy_to_user into a plain copy_to_user.  As si_code must
>   be massaged before being passed to userspace.
> 
> So remove these kernel internal si codes and make the kernel code simpler
> and more maintainable.
> 
> To replace these kernel internal magic si_codes introduce the helper
> function siginfo_layout, that takes a signal number and an si_code and
> computes which union member of siginfo is being used.  Have
> siginfo_layout return an enumeration so that gcc will have enough
> information to warn if a switch statement does not handle all of union
> members.
> 
> A couple of architectures have a messed up ABI that defines signal
> specific duplications of SI_USER which causes more special cases in
> siginfo_layout than I would like.  The good news is only problem
> architectures pay the cost.
> 
> Update all of the code that used the previous magic __SI_ values to
> use the new SIL_ values and to call siginfo_layout to get those
> values.  Escept where not all of the cases are handled remove the
> defaults in the switch statements so that if a new case is missed in
> the future the lack will show up at compile time.
> 
> Modify the code that copies siginfo si_code to userspace to just copy
> the value and not cast si_code to a short first.  The high bits are no
> longer used to hold a magic union member.
> 
> Fixup the siginfo header files to stop including the __SI_ values in
> their constants and for the headers that were missing it to properly
> update the number of si_codes for each signal type.
> 
> The fixes to copy_siginfo_from_user32 implementations has the
> interesting property that several of them perviously should never have
> worked as the __SI_ values they depended up where kernel internal.
> With that dependency gone those implementations should work much
> better.
> 
> The idea of not passing the __SI_ values out to userspace and then
> not reinserting them has been tested with criu and criu worked without
> changes.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>  arch/alpha/include/uapi/asm/siginfo.h    |   2 +-
>  arch/arm64/kernel/signal32.c             |  23 +++----
>  arch/blackfin/include/uapi/asm/siginfo.h |  30 +++++----
>  arch/frv/include/uapi/asm/siginfo.h      |   2 +-
>  arch/ia64/include/uapi/asm/siginfo.h     |  20 +++---
>  arch/ia64/kernel/signal.c                |  17 +++---
>  arch/mips/include/uapi/asm/siginfo.h     |   6 +-
>  arch/mips/kernel/signal32.c              |  19 +++---
>  arch/parisc/kernel/signal32.c            |  31 +++++-----
>  arch/powerpc/kernel/signal_32.c          |  20 +++---
>  arch/s390/kernel/compat_signal.c         |  32 +++++-----
>  arch/sparc/include/uapi/asm/siginfo.h    |   4 +-
>  arch/sparc/kernel/signal32.c             |  16 ++---
>  arch/tile/include/uapi/asm/siginfo.h     |   4 +-
>  arch/tile/kernel/compat_signal.c         |  18 +++---
>  arch/tile/kernel/traps.c                 |   1 -
>  arch/x86/kernel/signal_compat.c          |  21 +++----
>  fs/fcntl.c                               |   2 +-
>  fs/signalfd.c                            |  22 +++----
>  include/asm-generic/siginfo.h            |  22 ++++---
>  include/uapi/asm-generic/siginfo.h       | 102 ++++++++++++++-----------------
>  kernel/compat.c                          |   2 -
>  kernel/exit.c                            |   6 +-
>  kernel/ptrace.c                          |   6 +-
>  kernel/signal.c                          |  72 ++++++++++++++++------
>  25 files changed, 254 insertions(+), 246 deletions(-)
> 
<...>
> diff --git a/arch/tile/kernel/traps.c b/arch/tile/kernel/traps.c
> index 54804866f238..4433d1dc28e6 100644
> --- a/arch/tile/kernel/traps.c
> +++ b/arch/tile/kernel/traps.c
> @@ -188,7 +188,6 @@ static int special_ill(tile_bundle_bits bundle, int *sigp, int *codep)
>  
>  	/* Make it the requested signal. */
>  	*sigp = sig;
> -	*codep = code | __SI_FAULT;

Are you sure that we don't need to set codep here?

>  	return 1;
>  }
>