Re: [PATCH] information leak in sigaltstack

[Jakub Jelinek]

On Fri, Jul 31, 2009 at 05:28:40PM -0700, Linus Torvalds wrote:
> 
> [ Cc'ing jakub, since that code generation looks crappy, and I think he 
>   has worked on gcc memset(). I wonder if it's because we use -Os, and gcc 
>   tries to avoid one REX prefix on the 'stosq'.

Assuming we are talking about:
typedef __SIZE_TYPE__ size_t;
typedef struct sigaltstack { void *ss_sp; int ss_flags; size_t ss_size; }
stack_t;
void foo (stack_t *oss, void *sp)
{
  __builtin_memset (oss, 0, sizeof (*oss));
  oss->ss_sp = sp;
  oss->ss_flags = 6;
  oss->ss_size = 2; 
}

yes, it is because of -Os, rep stosq is longer than rep stosl.  For -O2 it
generates:
        movq    $0, 8(%rdi)
        movq    %rsi, (%rdi)
        movl    $6, 8(%rdi)
        movq    $2, 16(%rdi)
which still isn't perfect, but is much better.  GCC 4.4 newly has RTL DCE
improvements, so that at least the memcpy is optimized away if the structure
is filled completely after the memset, or is able to optimize away NULL/0
assignments after a memset to 0.  At -O2 when GCC decides to do the memset
piecewise it is easier to kill dead stores from the memset (the -O2 code
perhaps could be improved by the http://gcc.gnu.org/PR22141 patch which
hasn't been committed because it didn't show visible improvements and on
power6 even degraded performance).  At -Os when GCC decides during the
expand to use arch specific pattern for the memset it would be much harder
to handle it at the RTL level.  So the above should be ideally optimized
already at the tree level.

> diff --git a/kernel/signal.c b/kernel/signal.c
> index ccf1cee..b990dc8 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2455,6 +2455,9 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
>  	int error;
>  
>  	if (uoss) {
> +		/* Fill cracks around 'ss_flags' */
> +		if (__alignof__(oss.ss_flags) != __alignof__(oss))
> +			memset(&oss, 0, sizeof(oss));
>  		oss.ss_sp = (void __user *) current->sas_ss_sp;
>  		oss.ss_size = current->sas_ss_size;
>  		oss.ss_flags = sas_ss_flags(sp);

I'd say the test you want to do is
if (sizeof (oss.ss_sp) + sizeof (oss.ss_size) + sizeof (oss.ss_flags)
    != sizeof oss)
  memset (&oss, 0, sizeof oss);
(i.e. check whether the struct has any padding in it or not).

	Jakub
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/