| Message ID | 20200729115157.8519-1-yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Linux-kernel-mentees,v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() | expand |
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 <efremov@linux.com> 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 <efremov@linux.com> 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...
On Thu, Jul 30, 2020 at 8:10 PM Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org> 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/20210723221933.3431999-1-keescook@chromium.org
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));
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: stable@vger.kernel.org Suggested-by: Dan Carpenter <dan.carpenter@oracle.com> Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- Change in v2: - Remove inappropriate "Fixes:" tag. (Suggested by: Denis Efremov <efremov@linux.com>) 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(-)