raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
since it is initializing `cmd` by assignment, which may cause the compiler
to leave uninitialized holes in this structure. Fix it by using memcpy()
instead.
Cc: [email protected]
Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
$ pahole -C "floppy_raw_cmd" drivers/block/floppy.o
struct floppy_raw_cmd {
unsigned int flags; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
void * data; /* 8 8 */
char * kernel_data; /* 16 8 */
struct floppy_raw_cmd * next; /* 24 8 */
long int length; /* 32 8 */
long int phys_length; /* 40 8 */
int buffer_length; /* 48 4 */
unsigned char rate; /* 52 1 */
unsigned char cmd_count; /* 53 1 */
union {
struct {
unsigned char cmd[16]; /* 54 16 */
/* --- cacheline 1 boundary (64 bytes) was 6 bytes ago --- */
unsigned char reply_count; /* 70 1 */
unsigned char reply[16]; /* 71 16 */
}; /* 54 33 */
unsigned char fullcmd[33]; /* 54 33 */
}; /* 54 33 */
/* XXX 1 byte hole, try to pack */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
int track; /* 88 4 */
int resultcode; /* 92 4 */
int reserved1; /* 96 4 */
int reserved2; /* 100 4 */
/* size: 104, cachelines: 2, members: 14 */
/* sum members: 99, holes: 2, sum holes: 5 */
/* last cacheline: 40 bytes */
};
drivers/block/floppy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 09079aee8dc4..b8ea98f7a9cb 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3126,7 +3126,9 @@ static int raw_cmd_copyout(int cmd, void __user *param,
int ret;
while (ptr) {
- struct floppy_raw_cmd cmd = *ptr;
+ struct floppy_raw_cmd cmd;
+
+ memcpy(&cmd, ptr, sizeof(cmd));
cmd.next = NULL;
cmd.kernel_data = NULL;
ret = copy_to_user(param, &cmd, sizeof(cmd));
--
2.25.1
Hi,
On 7/28/20 5:19 PM, Peilin Ye wrote:
> raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
> since it is initializing `cmd` by assignment, which may cause the compiler
> to leave uninitialized holes in this structure. Fix it by using memcpy()
> instead.
>
> Cc: [email protected]
> Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
Reviewed-by: Denis Efremov <[email protected]>
ptr comes from raw_cmd_copyin and it should be ok to use memcpy.
Jens, could you please take this one to your 5.9 branch?
> ---
> $ pahole -C "floppy_raw_cmd" drivers/block/floppy.o
> struct floppy_raw_cmd {
> unsigned int flags; /* 0 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> void * data; /* 8 8 */
> char * kernel_data; /* 16 8 */
> struct floppy_raw_cmd * next; /* 24 8 */
> long int length; /* 32 8 */
> long int phys_length; /* 40 8 */
> int buffer_length; /* 48 4 */
> unsigned char rate; /* 52 1 */
> unsigned char cmd_count; /* 53 1 */
> union {
> struct {
> unsigned char cmd[16]; /* 54 16 */
> /* --- cacheline 1 boundary (64 bytes) was 6 bytes ago --- */
> unsigned char reply_count; /* 70 1 */
> unsigned char reply[16]; /* 71 16 */
> }; /* 54 33 */
> unsigned char fullcmd[33]; /* 54 33 */
> }; /* 54 33 */
>
> /* XXX 1 byte hole, try to pack */
>
> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> int track; /* 88 4 */
> int resultcode; /* 92 4 */
> int reserved1; /* 96 4 */
> int reserved2; /* 100 4 */
>
> /* size: 104, cachelines: 2, members: 14 */
> /* sum members: 99, holes: 2, sum holes: 5 */
> /* last cacheline: 40 bytes */
> };
>
It would be nice to add lkml links with discussion on the issue or
https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
in addition to pahole output.
> drivers/block/floppy.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 09079aee8dc4..b8ea98f7a9cb 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3126,7 +3126,9 @@ static int raw_cmd_copyout(int cmd, void __user *param,
> int ret;
>
> while (ptr) {
> - struct floppy_raw_cmd cmd = *ptr;
> + struct floppy_raw_cmd cmd;
> +
> + memcpy(&cmd, ptr, sizeof(cmd))> cmd.next = NULL;
> cmd.kernel_data = NULL;
> ret = copy_to_user(param, &cmd, sizeof(cmd));
>
Thanks,
Denis
On 7/28/20 5:19 PM, Peilin Ye wrote:
> raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
> since it is initializing `cmd` by assignment, which may cause the compiler
> to leave uninitialized holes in this structure. Fix it by using memcpy()
> instead.
>
> Cc: [email protected]
> Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
Nitpick, I would say this fix is not related to commit 2145e15e0557.
> Suggested-by: Dan Carpenter <[email protected]>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
On Wed, Jul 29, 2020 at 12:18:42PM +0300, Denis Efremov wrote:
>
>
> On 7/28/20 5:19 PM, Peilin Ye wrote:
> > raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
> > since it is initializing `cmd` by assignment, which may cause the compiler
> > to leave uninitialized holes in this structure. Fix it by using memcpy()
> > instead.
> >
> > Cc: [email protected]
> > Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
>
> Nitpick, I would say this fix is not related to commit 2145e15e0557.
I see, I will send a v2 soon. Thank you for reviewing the patch!
Peilin Ye
> > Suggested-by: Dan Carpenter <[email protected]>
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Peilin Ye <[email protected]>
raw_cmd_copyout() is potentially copying uninitialized kernel stack memory
since it is initializing `cmd` by assignment, which may cause the compiler
to leave uninitialized holes in this structure. Fix it by using memcpy()
instead.
Cc: [email protected]
Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Change in v2:
- Remove inappropriate "Fixes:" tag. (Suggested by: Denis Efremov
<[email protected]>)
Reference: https://lwn.net/Articles/417989/
$ pahole -C "floppy_raw_cmd" drivers/block/floppy.o
struct floppy_raw_cmd {
unsigned int flags; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
void * data; /* 8 8 */
char * kernel_data; /* 16 8 */
struct floppy_raw_cmd * next; /* 24 8 */
long int length; /* 32 8 */
long int phys_length; /* 40 8 */
int buffer_length; /* 48 4 */
unsigned char rate; /* 52 1 */
unsigned char cmd_count; /* 53 1 */
union {
struct {
unsigned char cmd[16]; /* 54 16 */
/* --- cacheline 1 boundary (64 bytes) was 6 bytes ago --- */
unsigned char reply_count; /* 70 1 */
unsigned char reply[16]; /* 71 16 */
}; /* 54 33 */
unsigned char fullcmd[33]; /* 54 33 */
}; /* 54 33 */
/* XXX 1 byte hole, try to pack */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
int track; /* 88 4 */
int resultcode; /* 92 4 */
int reserved1; /* 96 4 */
int reserved2; /* 100 4 */
/* size: 104, cachelines: 2, members: 14 */
/* sum members: 99, holes: 2, sum holes: 5 */
/* last cacheline: 40 bytes */
};
drivers/block/floppy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 09079aee8dc4..b8ea98f7a9cb 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3126,7 +3126,9 @@ static int raw_cmd_copyout(int cmd, void __user *param,
int ret;
while (ptr) {
- struct floppy_raw_cmd cmd = *ptr;
+ struct floppy_raw_cmd cmd;
+
+ memcpy(&cmd, ptr, sizeof(cmd));
cmd.next = NULL;
cmd.kernel_data = NULL;
ret = copy_to_user(param, &cmd, sizeof(cmd));
--
2.25.1
Argh... This isn't right still. The "ptr" comes from raw_cmd_copyin()
ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
The struct hole could still be uninitialized from kmalloc() and instead
of from the stack. Smatch is only looking for the common stack info
leaks and doesn't worn about holes in kmalloc()ed memory.
regards,
dan carpenter
On 7/29/20 3:58 PM, Dan Carpenter wrote:
> Argh... This isn't right still. The "ptr" comes from raw_cmd_copyin()
>
> ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
>
copy_from_user overwrites the padding bytes:
ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
if (!ptr)
return -ENOMEM;
*rcmd = ptr;
ret = copy_from_user(ptr, param, sizeof(*ptr));
I think memcpy should be safe in this patch.
I've decided to dig a bit into the issue and to run some tests.
Here are my observations:
$ cat test.c
#include <stdio.h>
#include <string.h>
#define __user
struct floppy_raw_cmd {
unsigned int flags;
void __user *data;
char *kernel_data; /* location of data buffer in the kernel */
struct floppy_raw_cmd *next; /* used for chaining of raw cmd's
* within the kernel */
long length; /* in: length of dma transfer. out: remaining bytes */
long phys_length; /* physical length, if different from dma length */
int buffer_length; /* length of allocated buffer */
unsigned char rate;
#define FD_RAW_CMD_SIZE 16
#define FD_RAW_REPLY_SIZE 16
#define FD_RAW_CMD_FULLSIZE (FD_RAW_CMD_SIZE + 1 + FD_RAW_REPLY_SIZE)
unsigned char cmd_count;
union {
struct {
unsigned char cmd[FD_RAW_CMD_SIZE];
unsigned char reply_count;
unsigned char reply[FD_RAW_REPLY_SIZE];
};
unsigned char fullcmd[FD_RAW_CMD_FULLSIZE];
};
int track;
int resultcode;
int reserved1;
int reserved2;
};
void __attribute__((noinline)) stack_alloc()
{
struct floppy_raw_cmd stack;
memset(&stack, 0xff, sizeof(struct floppy_raw_cmd));
asm volatile ("" ::: "memory");
}
int __attribute__((noinline)) test(struct floppy_raw_cmd *ptr)
{
struct floppy_raw_cmd cmd = *ptr;
int i;
for (i = 0; i < sizeof(struct floppy_raw_cmd); ++i) {
if (((char *)&cmd)[i]) {
printf("leak[%d]\n", i);
return i;
}
}
return 0;
}
int main(int argc, char *argv[])
{
struct floppy_raw_cmd zero;
memset(&zero, 0, sizeof(struct floppy_raw_cmd));
// For selfcheck uncomment:
// zero.resultcode = 1;
stack_alloc();
return test(&zero);
}
Next, I've prepared containers with gcc 4.8 5 6 7 8 9 10 versions with this
tool (https://github.com/a13xp0p0v/kernel-build-containers).
And checked for leaks on x86_64 with the script test.sh
$ cat test.sh
#!/bin/bash
for i in 4.8 5 6 7 8 9 10
do
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc test.c; ./a.out'
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O2 test.c; ./a.out'
./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O3 test.c; ./a.out'
done
No leaks reported. Is it really possible this this kind of init, i.e. cmd = *ptr?
https://lwn.net/Articles/417989/ (December 1, 2010).
GCC 4.9.4 released [2016-08-03]
Maybe this behavior changed.
https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
Reports for >= 4.7, < 8.0 version. But I can't find a word about this
kind of inits: cmd = *ptr.
Thanks,
Denis
On Wed, Jul 29, 2020 at 04:22:41PM +0300, Denis Efremov wrote:
>
> On 7/29/20 3:58 PM, Dan Carpenter wrote:
> > Argh... This isn't right still. The "ptr" comes from raw_cmd_copyin()
> >
> > ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
> >
>
> copy_from_user overwrites the padding bytes:
> ptr = kmalloc(sizeof(struct floppy_raw_cmd), GFP_KERNEL);
> if (!ptr)
> return -ENOMEM;
> *rcmd = ptr;
> ret = copy_from_user(ptr, param, sizeof(*ptr));
>
> I think memcpy should be safe in this patch.
Oh yeah. You're right. My bad. I just saw the:
ptr->next = NULL;
ptr->buffer_length = 0;
ptr->kernel_data = NULL;
Assignments and I missed the copy_from_user.
regards,
dan carpenter
> On Wed, Jul 29, 2020 at 3:22 PM Denis Efremov <[email protected]> wrote:
> And checked for leaks on x86_64 with the script test.sh
> $ cat test.sh
> #!/bin/bash
>
> for i in 4.8 5 6 7 8 9 10
> do
> ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc test.c; ./a.out'
> ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O2 test.c; ./a.out'
> ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O3 test.c; ./a.out'
> done
>
> No leaks reported. Is it really possible this this kind of init, i.e. cmd = *ptr?
The problem is that the behavior is dependent not just on the compiler
version but
also optimization flags, target architecture and specific structure
layouts. Most
of the time, both gcc and clang will initialize the whole structure
rather than just
the individual members, but you still can't be sure that this is true
for all configurations
that this code runs on, except by using CONFIG_GCC_PLUGIN_STRUCTLEAK.
Kees pointed me to the lib/test_stackinit.c file in the kernel in which he has
collected a number of combinations that are known to trigger the problem.
What I see there though are only cases of struct initializers like
struct test_big_hole var = { .one = arg->one, .two=arg->two, .three
= arg->three, .four = arg->four };
but not the syntax used in the floppy driver:
struct test_big_hole var = *arg;
or the a constructor like
struct test_big_hole var;
var = (struct test_big_hole){ .one = arg->one, .two=arg->two, .three
= arg->three, .four = arg->four };
Kees, do you know whether those two would behave differently?
Would it make sense to also check for those, or am I perhaps
misreading your code and it already gets checked?
Arnd
On Thu, Jul 30, 2020 at 10:11:07AM +0200, Arnd Bergmann wrote:
> > On Wed, Jul 29, 2020 at 3:22 PM Denis Efremov <[email protected]> wrote:
>
> > And checked for leaks on x86_64 with the script test.sh
> > $ cat test.sh
> > #!/bin/bash
> >
> > for i in 4.8 5 6 7 8 9 10
> > do
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc test.c; ./a.out'
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O2 test.c; ./a.out'
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O3 test.c; ./a.out'
> > done
> >
> > No leaks reported. Is it really possible this this kind of init, i.e. cmd = *ptr?
>
> The problem is that the behavior is dependent not just on the compiler
> version but
> also optimization flags, target architecture and specific structure
> layouts. Most
> of the time, both gcc and clang will initialize the whole structure
> rather than just
> the individual members, but you still can't be sure that this is true
> for all configurations
> that this code runs on, except by using CONFIG_GCC_PLUGIN_STRUCTLEAK.
>
> Kees pointed me to the lib/test_stackinit.c file in the kernel in which he has
> collected a number of combinations that are known to trigger the problem.
>
> What I see there though are only cases of struct initializers like
>
> struct test_big_hole var = { .one = arg->one, .two=arg->two, .three
> = arg->three, .four = arg->four };
test_stackinit.c intended to use six cases (where "full" is in the sense
of "all members are named", this is intentionally testing the behavior
of padding hole initialization):
full static initialization:
= { .one = 0,
.two = 0,
.three = 0,
.four = 0,
};
partial static init:
= { .two = 0, };
full dynamic init:
= { .one = arg->one,
.two = arg->two,
.three = arg->three,
.four = arg->four,
};
partial dynamic init:
= { .two = arg->two, };
full runtime init:
var.one = 0;
var.two = 0;
var.three = 0;
memset(&var.four, 0, sizeof(var.four));
partial runtime init:
var.two = 0;
(It seems in refactoring I botched the "full static initialization"
case, which I'll go fix separately.)
> but not the syntax used in the floppy driver:
>
> struct test_big_hole var = *arg;
So this one is a "whole structure copy" which I didn't have any tests
for, since I'd (perhaps inappropriately) assumed would be accomplished
with memcpy() internally, which means the incoming "*arg"'s padding holes
would be copied as-is. If the compiler is actually doing per-member copies
and leaving holes in "var" untouched, that's unexpected, so clearly that
needs to be added to test_stackinit.c! :)
> or the a constructor like
>
> struct test_big_hole var;
> var = (struct test_big_hole){ .one = arg->one, .two=arg->two, .three
> = arg->three, .four = arg->four };
>
> Kees, do you know whether those two would behave differently?
> Would it make sense to also check for those, or am I perhaps
> misreading your code and it already gets checked?
I *think* the above constructor would be covered under "full runtime
init", but it does also seem likely it would be handled similarly to
the "whole structure copy" in the previous example. I will go add more
tests...
--
Kees Cook
On Thu, Jul 30, 2020 at 8:10 PM Kees Cook <[email protected]> wrote:
> On Thu, Jul 30, 2020 at 10:11:07AM +0200, Arnd Bergmann wrote:
>
> test_stackinit.c intended to use six cases (where "full" is in the sense
> of "all members are named", this is intentionally testing the behavior
> of padding hole initialization):
Ok, so I read that correctly, thanks for confirming.
> >
> > struct test_big_hole var = *arg;
>
> So this one is a "whole structure copy" which I didn't have any tests
> for, since I'd (perhaps inappropriately) assumed would be accomplished
> with memcpy() internally, which means the incoming "*arg"'s padding holes
> would be copied as-is. If the compiler is actually doing per-member copies
> and leaving holes in "var" untouched, that's unexpected, so clearly that
> needs to be added to test_stackinit.c! :)
For some reason I remembered this not turning into a memcpy()
somewhere, but I can't reproduce it in any of my recent attempts,
just like what Denis found.
> > or the a constructor like
> >
> > struct test_big_hole var;
> > var = (struct test_big_hole){ .one = arg->one, .two=arg->two, .three
> > = arg->three, .four = arg->four };
> >
> > Kees, do you know whether those two would behave differently?
> > Would it make sense to also check for those, or am I perhaps
> > misreading your code and it already gets checked?
>
> I *think* the above constructor would be covered under "full runtime
> init", but it does also seem likely it would be handled similarly to
> the "whole structure copy" in the previous example.
I would assume that at least with C99 it is more like the
"whole structure copy", based on the standard language of
"The value of the compound literal is that of an unnamed
object initialized by the initializer list. If the compound literal
occurs outside the body of a function, the object has static
storage duration; otherwise, it has automatic storage duration
associated with the enclosing block."
> I will go add more tests...
Thanks!
Arnd
On Thu, Jul 30, 2020 at 10:45:02PM +0200, Arnd Bergmann wrote:
> On Thu, Jul 30, 2020 at 8:10 PM Kees Cook <[email protected]> wrote:
> > On Thu, Jul 30, 2020 at 10:11:07AM +0200, Arnd Bergmann wrote:
> >
> > test_stackinit.c intended to use six cases (where "full" is in the sense
> > of "all members are named", this is intentionally testing the behavior
> > of padding hole initialization):
>
> Ok, so I read that correctly, thanks for confirming.
>
> > >
> > > struct test_big_hole var = *arg;
> >
> > So this one is a "whole structure copy" which I didn't have any tests
> > for, since I'd (perhaps inappropriately) assumed would be accomplished
> > with memcpy() internally, which means the incoming "*arg"'s padding holes
> > would be copied as-is. If the compiler is actually doing per-member copies
> > and leaving holes in "var" untouched, that's unexpected, so clearly that
> > needs to be added to test_stackinit.c! :)
>
> For some reason I remembered this not turning into a memcpy()
> somewhere, but I can't reproduce it in any of my recent attempts,
> just like what Denis found.
>
> > > or the a constructor like
> > >
> > > struct test_big_hole var;
> > > var = (struct test_big_hole){ .one = arg->one, .two=arg->two, .three
> > > = arg->three, .four = arg->four };
> > >
> > > Kees, do you know whether those two would behave differently?
> > > Would it make sense to also check for those, or am I perhaps
> > > misreading your code and it already gets checked?
> >
> > I *think* the above constructor would be covered under "full runtime
> > init", but it does also seem likely it would be handled similarly to
> > the "whole structure copy" in the previous example.
>
> I would assume that at least with C99 it is more like the
> "whole structure copy", based on the standard language of
>
> "The value of the compound literal is that of an unnamed
> object initialized by the initializer list. If the compound literal
> occurs outside the body of a function, the object has static
> storage duration; otherwise, it has automatic storage duration
> associated with the enclosing block."
>
> > I will go add more tests...
>
> Thanks!
Well, nearly exactly a year later, I've finally done this. :P
https://lore.kernel.org/lkml/[email protected]
--
Kees Cook