From: Denis Efremov <efremov@linux.com>
To: Dan Carpenter <dan.carpenter@oracle.com>,
Peilin Ye <yepeilin.cs@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>, Arnd Bergmann <arnd@arndb.de>,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
Date: Wed, 29 Jul 2020 16:22:41 +0300 [thread overview]
Message-ID: <f2cf6137-987a-ab41-d88a-6828d46c255f@linux.com> (raw)
In-Reply-To: <20200729125820.GB1840@kadam>
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
next prev parent reply other threads:[~2020-07-29 13:22 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
2021-07-23 22:22 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2cf6137-987a-ab41-d88a-6828d46c255f@linux.com \
--to=efremov@linux.com \
--cc=arnd@arndb.de \
--cc=axboe@kernel.dk \
--cc=dan.carpenter@oracle.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=yepeilin.cs@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).