* [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
@ 2019-10-26 19:46 Joe Perches
2019-10-27 5:47 ` Julia Lawall
2019-10-28 7:18 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Joe Perches @ 2019-10-26 19:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: LKML, Dan Carpenter, Julia Lawall, Thomas Gleixner
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-26 19:46 [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user Joe Perches
@ 2019-10-27 5:47 ` Julia Lawall
2019-10-27 22:47 ` Joe Perches
2019-10-28 7:18 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2019-10-27 5:47 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, LKML, Dan Carpenter, Julia Lawall, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-27 5:47 ` Julia Lawall
@ 2019-10-27 22:47 ` Joe Perches
2019-10-28 7:30 ` Dan Carpenter
2019-10-28 18:58 ` Kees Cook
0 siblings, 2 replies; 7+ messages in thread
From: Joe Perches @ 2019-10-27 22:47 UTC (permalink / raw)
To: Julia Lawall
Cc: Andrew Morton, LKML, Dan Carpenter, Thomas Gleixner, Kees Cook, zhanglin
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-26 19:46 [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user Joe Perches
2019-10-27 5:47 ` Julia Lawall
@ 2019-10-28 7:18 ` Dan Carpenter
2019-10-28 8:08 ` Joe Perches
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-10-28 7:18 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, LKML, Dan Carpenter, Julia Lawall, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-27 22:47 ` Joe Perches
@ 2019-10-28 7:30 ` Dan Carpenter
2019-10-28 18:58 ` Kees Cook
1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-10-28 7:30 UTC (permalink / raw)
To: Joe Perches
Cc: Julia Lawall, Andrew Morton, LKML, Dan Carpenter,
Thomas Gleixner, Kees Cook, zhanglin
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-28 7:18 ` Dan Carpenter
@ 2019-10-28 8:08 ` Joe Perches
0 siblings, 0 replies; 7+ messages in thread
From: Joe Perches @ 2019-10-28 8:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andrew Morton, LKML, Dan Carpenter, Julia Lawall, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user
2019-10-27 22:47 ` Joe Perches
2019-10-28 7:30 ` Dan Carpenter
@ 2019-10-28 18:58 ` Kees Cook
1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2019-10-28 18:58 UTC (permalink / raw)
To: Joe Perches
Cc: Julia Lawall, Andrew Morton, LKML, Dan Carpenter,
Thomas Gleixner, zhanglin
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-28 18:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-26 19:46 [PATCH] kernel: sys.c: Avoid copying possible padding bytes in copy_to_user Joe Perches
2019-10-27 5:47 ` Julia Lawall
2019-10-27 22:47 ` Joe Perches
2019-10-28 7:30 ` Dan Carpenter
2019-10-28 18:58 ` Kees Cook
2019-10-28 7:18 ` Dan Carpenter
2019-10-28 8:08 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.