Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
@ 2020-07-28 14:19 Peilin Ye
  2020-07-29  9:07 ` Denis Efremov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Peilin Ye @ 2020-07-28 14:19 UTC (permalink / raw)
  To: Denis Efremov, Jens Axboe
  Cc: Peilin Ye, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-block, linux-kernel

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
Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
$ 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-28 14:19 [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() Peilin Ye
@ 2020-07-29  9:07 ` Denis Efremov
  2020-07-29  9:18 ` Denis Efremov
  2020-07-29 11:51 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  2 siblings, 0 replies; 11+ messages in thread
From: Denis Efremov @ 2020-07-29  9:07 UTC (permalink / raw)
  To: Peilin Ye, Jens Axboe
  Cc: Arnd Bergmann, linux-kernel, linux-block, linux-kernel-mentees,
	Dan Carpenter

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: stable@vger.kernel.org
> Fixes: 2145e15e0557 ("floppy: don't write kernel-only members to FDRAWCMD ioctl output")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

Reviewed-by: Denis Efremov <efremov@linux.com>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-28 14:19 [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() Peilin Ye
  2020-07-29  9:07 ` Denis Efremov
@ 2020-07-29  9:18 ` Denis Efremov
  2020-07-29  9:46   ` Peilin Ye
  2020-07-29 11:51 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
  2 siblings, 1 reply; 11+ messages in thread
From: Denis Efremov @ 2020-07-29  9:18 UTC (permalink / raw)
  To: Peilin Ye, Jens Axboe
  Cc: Arnd Bergmann, linux-kernel, linux-block, linux-kernel-mentees,
	Dan Carpenter



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: stable@vger.kernel.org
> 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 <dan.carpenter@oracle.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-29  9:18 ` Denis Efremov
@ 2020-07-29  9:46   ` Peilin Ye
  0 siblings, 0 replies; 11+ messages in thread
From: Peilin Ye @ 2020-07-29  9:46 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Arnd Bergmann, linux-kernel, linux-block, linux-kernel-mentees,
	Dan Carpenter

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: stable@vger.kernel.org
> > 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 <dan.carpenter@oracle.com>
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-28 14:19 [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() Peilin Ye
  2020-07-29  9:07 ` Denis Efremov
  2020-07-29  9:18 ` Denis Efremov
@ 2020-07-29 11:51 ` Peilin Ye
  2020-07-29 12:58   ` Dan Carpenter
  2 siblings, 1 reply; 11+ messages in thread
From: Peilin Ye @ 2020-07-29 11:51 UTC (permalink / raw)
  To: Denis Efremov, Jens Axboe
  Cc: Peilin Ye, Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-block, linux-kernel

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(-)

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-29 11:51 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
@ 2020-07-29 12:58   ` Dan Carpenter
  2020-07-29 13:22     ` Denis Efremov
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2020-07-29 12:58 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Denis Efremov, Jens Axboe, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, linux-block, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-29 12:58   ` Dan Carpenter
@ 2020-07-29 13:22     ` Denis Efremov
  2020-07-29 13:42       ` Dan Carpenter
  2020-07-30  8:11       ` Arnd Bergmann
  0 siblings, 2 replies; 11+ messages in thread
From: Denis Efremov @ 2020-07-29 13:22 UTC (permalink / raw)
  To: Dan Carpenter, Peilin Ye
  Cc: Jens Axboe, Arnd Bergmann, linux-kernel, linux-block,
	linux-kernel-mentees


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-29 13:22     ` Denis Efremov
@ 2020-07-29 13:42       ` Dan Carpenter
  2020-07-30  8:11       ` Arnd Bergmann
  1 sibling, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2020-07-29 13:42 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Peilin Ye, Jens Axboe, Arnd Bergmann, linux-kernel, linux-block,
	linux-kernel-mentees

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-29 13:22     ` Denis Efremov
  2020-07-29 13:42       ` Dan Carpenter
@ 2020-07-30  8:11       ` Arnd Bergmann
  2020-07-30 18:10         ` Kees Cook
  1 sibling, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2020-07-30  8:11 UTC (permalink / raw)
  To: Denis Efremov
  Cc: Dan Carpenter, Peilin Ye, Jens Axboe, linux-kernel, linux-block,
	linux-kernel-mentees, Kees Cook

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-30  8:11       ` Arnd Bergmann
@ 2020-07-30 18:10         ` Kees Cook
  2020-07-30 20:45           ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-07-30 18:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Denis Efremov, Dan Carpenter, Peilin Ye, Jens Axboe,
	linux-kernel, linux-block, linux-kernel-mentees

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...

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
  2020-07-30 18:10         ` Kees Cook
@ 2020-07-30 20:45           ` Arnd Bergmann
  0 siblings, 0 replies; 11+ messages in thread
From: Arnd Bergmann @ 2020-07-30 20:45 UTC (permalink / raw)
  To: Kees Cook
  Cc: Denis Efremov, Dan Carpenter, Peilin Ye, Jens Axboe,
	linux-kernel, linux-block, linux-kernel-mentees

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 14:19 [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() Peilin Ye
2020-07-29  9:07 ` Denis Efremov
2020-07-29  9:18 ` Denis Efremov
2020-07-29  9:46   ` Peilin Ye
2020-07-29 11:51 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
2020-07-29 12:58   ` Dan Carpenter
2020-07-29 13:22     ` Denis Efremov
2020-07-29 13:42       ` Dan Carpenter
2020-07-30  8:11       ` Arnd Bergmann
2020-07-30 18:10         ` Kees Cook
2020-07-30 20:45           ` Arnd Bergmann

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git