linux-kernel - Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <92212e57d45f4410be654183f5dcb1e98d636ef2.camel@perches.com>
Date:   Sun, 27 Oct 2019 15:47:21 -0700
From:   Joe Perches <joe@...ches.com>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Dan Carpenter <error27@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kees Cook <keescook@...omium.org>,
        zhanglin <zhang.lin16@....com.cn>
Subject: Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in
 copy_to_user

On Sun, 2019-10-27 at 06:47 +0100, Julia Lawall wrote:
> 
> On Sat, 26 Oct 2019, Joe Perches wrote:
> 
> > Initialization is not guaranteed to zero padding bytes so
> > use an explicit memset instead to avoid leaking any kernel
> > content in any possible padding bytes.
> 
> Here is an extract of an email that I sent to Kees at one point that left
> me unsure about what should be done about these situations:
> 
> From Kees:
> 
>     The only way to correctly handle this is:
> 
>     memset(&instance, 0, sizeof(instance));
>     instance.one = 1;
> 
> From me:
> 
> Actually, this document:
> 
> https://wiki.sei.cmu.edu/confluence/display/c/DCL39-C.+Avoid+information+leakage+when+passing+a+structure+across+a+trust+boundary
> 
> says that memset is a "noncompliant solution".  They suggest declaring the
> structure as packed, as well as some other more unpleasant solutions.
> Their point is that 1 will be sitting in a register, and the assignment at
> least might copy the upper bytes of the register into the padding space.

It took me a minute to understand why, but it
is true and possible.

> Is the memset solution nevertheless what is always wanted in the kernel
> when there is padding?

I think yes as at least it makes it consistent.

>From the link above, as I understand the __user
gcc extension here
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c61f13eaa1ee17728c41370100d2d45c254ce76f

gcc does not clear padding from initialized structs
marked with __user.

Perhaps adding yet another attribute to struct definitions
and another gcc extension could help.

Perhaps add something like

	#define __uapi __attribute__((__uapi__))

and mark the struct definitions in include/uapi like:

struct ethtool_wolinfo {
	__u32	cmd;
	__u32	supported;
	__u32	wolopts;
	__u8	sopass[SOPASS_MAX];
} __uapi;

so that gcc could make sure any struct padding
is also zeroed if initialized.

Though that doesn't force the compiler to not
perform the possible register optimization shown
in the first document above.