linux-kernel - Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2cf6137-987a-ab41-d88a-6828d46c255f@linux.com>
Date:   Wed, 29 Jul 2020 16:22:41 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Peilin Ye <yepeilin.cs@...il.com>
Cc:     Jens Axboe <axboe@...nel.dk>, Arnd Bergmann <arnd@...db.de>,
        linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent
 kernel-infoleak in raw_cmd_copyout()


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