[PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Joe Perches]

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.

Signed-off-by: Joe Perches <joe@perches.com>
---
 kernel/sys.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index a611d1..3459a5 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1279,11 +1279,13 @@ SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
 
 SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
 {
-	struct oldold_utsname tmp = {};
+	struct oldold_utsname tmp;
 
 	if (!name)
 		return -EFAULT;
 
+	memset(&tmp, 0, sizeof(tmp));
+
 	down_read(&uts_sem);
 	memcpy(&tmp.sysname, &utsname()->sysname, __OLD_UTS_LEN);
 	memcpy(&tmp.nodename, &utsname()->nodename, __OLD_UTS_LEN);

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Julia Lawall]

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.

-------------------------

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

thanks,
julia

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Joe Perches]

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.

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Dan Carpenter]

We should be able to use memzero_explicit(), right?

The fact that we memset() can't be used to prevent information leaks has
always worried me.  Everyone predicted that we would have bugs like this
where memset doesn't work as expected.

regards,
dan carpenter

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Kees Cook]

On Sun, Oct 27, 2019 at 03:47:21PM -0700, Joe Perches wrote:
> 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.

It seems to depend on how complete the initialization is and/or how
large the structure is. AFAICT, based on the tests I wrote[1], if
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF (or ..._ALL) are used, even padding
will get initialized as long as things are in memory. (And the same is
true with Clang under CONFIG_INIT_STACK_ALL.)

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

Right. This is the only case where things aren't clear. I haven't been
able to build a test where "store in registers" behavior is tripped.

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/test_stackinit.c

-- 
Kees Cook

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Dan Carpenter]

On Sat, Oct 26, 2019 at 12:46:08PM -0700, 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.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  kernel/sys.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a611d1..3459a5 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1279,11 +1279,13 @@ SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
>  
>  SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
>  {
> -	struct oldold_utsname tmp = {};
> +	struct oldold_utsname tmp;

oldold_utsname doesn't have an struct holes.  It looks like this:

struct oldold_utsname {
        char sysname[9];
        char nodename[9];
        char release[9];
        char version[9];
        char machine[9];
};

regards,
dan carpenter

Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user

[Joe Perches]

On Mon, 2019-10-28 at 10:18 +0300, Dan Carpenter wrote:
> On Sat, Oct 26, 2019 at 12:46:08PM -0700, 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.
[]
> > diff --git a/kernel/sys.c b/kernel/sys.c
[]
> > @@ -1279,11 +1279,13 @@ SYSCALL_DEFINE1(uname, struct old_utsname __user *, name)
> >  
> >  SYSCALL_DEFINE1(olduname, struct oldold_utsname __user *, name)
> >  {
> > -	struct oldold_utsname tmp = {};
> > +	struct oldold_utsname tmp;
> 
> oldold_utsname doesn't have an struct holes.  It looks like this:

It's not struct holes that could be a problem.
It's possible struct padding after the last element.

> struct oldold_utsname {
>         char sysname[9];
>         char nodename[9];
>         char release[9];
>         char version[9];
>         char machine[9];
> };

Nominally 45 bytes.

A compiler _could_ pad to 48 for arbitrary alignment.
gcc does not pad and the struct size actually is 45
so for gcc (and I did not check clang), it's not a
real problem.

The patch still is a possible trivial improvement as
the memset is not done when name is NULL.