* Security hole in cros_ec_dev.c on 32bit chrome hosts
@ 2016-03-01 20:33 Alan Cox
2016-03-03 5:58 ` [PATCH] platform/chrome: cros_ec_dev - Fix security issue Gwendal Grignou
0 siblings, 1 reply; 8+ messages in thread
From: Alan Cox @ 2016-03-01 20:33 UTC (permalink / raw)
To: linux-kernel, gwendal, javier.martinez
This was reported to Google on Feb 2nd with no action but an
acknowledgement.
Making public since so as we are close to release
/* Ioctls */
static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
{
long ret;
struct cros_ec_command u_cmd;
struct cros_ec_command *s_cmd;
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
return -EFAULT;
s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
GFP_KERNEL);
Pass u_cmd.insize as a very large value so that it overflows with the
sizeof to a small number which we kmalloc
if (!s_cmd)
return -ENOMEM;
and copy u_cmd.outsize bytes into it.
if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) {
ret = -EFAULT;
goto exit;
}
Alan
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] platform/chrome: cros_ec_dev - Fix security issue
2016-03-01 20:33 Security hole in cros_ec_dev.c on 32bit chrome hosts Alan Cox
@ 2016-03-03 5:58 ` Gwendal Grignou
2016-03-03 18:35 ` Randy Dunlap
0 siblings, 1 reply; 8+ messages in thread
From: Gwendal Grignou @ 2016-03-03 5:58 UTC (permalink / raw)
To: olofj, alan, javier.martinez; +Cc: linux-kernel
Add a check to prevent memory scribbe when sending an ioctl with .insize
set so large that memory allocation argument overflows.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/platform/chrome/cros_ec_dev.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index d45cd25..86d6373 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -131,13 +131,23 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
{
long ret;
+ size_t data_size;
struct cros_ec_command u_cmd;
struct cros_ec_command *s_cmd;
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
return -EFAULT;
- s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
+ /*
+ * Prevent mallicious attack where .inside is so big that amount
+ * kmalloc'ed rollover, allowing memcpy to write beyond the allocated
+ * space.
+ */
+ data_size = max(u_cmd.outsize, u_cmd.insize);
+ if (data_size + sizeof(*s_cmd) < data_size)
+ return -EINVAL;
+
+ s_cmd = kmalloc(sizeof(*s_cmd) + data_size,
GFP_KERNEL);
if (!s_cmd)
return -ENOMEM;
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_dev - Fix security issue
2016-03-03 5:58 ` [PATCH] platform/chrome: cros_ec_dev - Fix security issue Gwendal Grignou
@ 2016-03-03 18:35 ` Randy Dunlap
2016-03-03 19:00 ` [PATCH v2] " Gwendal Grignou
0 siblings, 1 reply; 8+ messages in thread
From: Randy Dunlap @ 2016-03-03 18:35 UTC (permalink / raw)
To: Gwendal Grignou, olofj, alan, javier.martinez; +Cc: linux-kernel
On 03/02/16 21:58, Gwendal Grignou wrote:
> Add a check to prevent memory scribbe when sending an ioctl with .insize
scribble
> set so large that memory allocation argument overflows.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
> drivers/platform/chrome/cros_ec_dev.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index d45cd25..86d6373 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -131,13 +131,23 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
> static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
> {
> long ret;
> + size_t data_size;
> struct cros_ec_command u_cmd;
> struct cros_ec_command *s_cmd;
>
> if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
> return -EFAULT;
>
> - s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
> + /*
> + * Prevent mallicious attack where .inside is so big that amount
malicious .insize
> + * kmalloc'ed rollover, allowing memcpy to write beyond the allocated
> + * space.
> + */
> + data_size = max(u_cmd.outsize, u_cmd.insize);
> + if (data_size + sizeof(*s_cmd) < data_size)
> + return -EINVAL;
> +
> + s_cmd = kmalloc(sizeof(*s_cmd) + data_size,
> GFP_KERNEL);
> if (!s_cmd)
> return -ENOMEM;
>
--
~Randy
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue
2016-03-03 18:35 ` Randy Dunlap
@ 2016-03-03 19:00 ` Gwendal Grignou
2016-03-06 20:11 ` Olof Johansson
0 siblings, 1 reply; 8+ messages in thread
From: Gwendal Grignou @ 2016-03-03 19:00 UTC (permalink / raw)
To: rdunlap, olofj, alan, cernekee; +Cc: linux-kernel
Add a check to prevent memory scribble when sending an ioctl with .insize
set so large that memory allocation argument overflows.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/platform/chrome/cros_ec_dev.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index d45cd25..0b2f730 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -131,13 +131,23 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
{
long ret;
+ size_t data_size;
struct cros_ec_command u_cmd;
struct cros_ec_command *s_cmd;
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
return -EFAULT;
- s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
+ /*
+ * Prevent malicious attack where .insize is so big that amount
+ * kmalloc'ed rollover, allowing memcpy to write beyond the allocated
+ * space.
+ */
+ data_size = max(u_cmd.outsize, u_cmd.insize);
+ if (data_size + sizeof(*s_cmd) < data_size)
+ return -EINVAL;
+
+ s_cmd = kmalloc(sizeof(*s_cmd) + data_size,
GFP_KERNEL);
if (!s_cmd)
return -ENOMEM;
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue
2016-03-03 19:00 ` [PATCH v2] " Gwendal Grignou
@ 2016-03-06 20:11 ` Olof Johansson
2016-03-08 17:02 ` Gwendal Grignou
0 siblings, 1 reply; 8+ messages in thread
From: Olof Johansson @ 2016-03-06 20:11 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: rdunlap, olofj, alan, cernekee, linux-kernel
Hi,
On Thu, Mar 03, 2016 at 11:00:13AM -0800, Gwendal Grignou wrote:
> Add a check to prevent memory scribble when sending an ioctl with .insize
> set so large that memory allocation argument overflows.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
> drivers/platform/chrome/cros_ec_dev.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index d45cd25..0b2f730 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -131,13 +131,23 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
> static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
> {
> long ret;
> + size_t data_size;
> struct cros_ec_command u_cmd;
> struct cros_ec_command *s_cmd;
>
> if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
> return -EFAULT;
>
> - s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
> + /*
> + * Prevent malicious attack where .insize is so big that amount
> + * kmalloc'ed rollover, allowing memcpy to write beyond the allocated
> + * space.
> + */
> + data_size = max(u_cmd.outsize, u_cmd.insize);
> + if (data_size + sizeof(*s_cmd) < data_size)
> + return -EINVAL;
> +
> + s_cmd = kmalloc(sizeof(*s_cmd) + data_size,
> GFP_KERNEL);
This test does work, but it's sort of silly to even try to allow almost 4GB of
allocation here.
How about you introduce a reasonable max size for a transaction instead
(256K?), and compare data_size with that? Might want to check with the EC folks
what they expect larges transactions to be from their side, and go with
a margin above that.
Also, in your commit message you should refer to the CVE this fixes.
-Olof
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue
2016-03-06 20:11 ` Olof Johansson
@ 2016-03-08 17:02 ` Gwendal Grignou
2016-03-08 17:13 ` [PATCH v3] " Gwendal Grignou
0 siblings, 1 reply; 8+ messages in thread
From: Gwendal Grignou @ 2016-03-08 17:02 UTC (permalink / raw)
To: Olof Johansson
Cc: Gwendal Grignou, rdunlap, Olof Johansson, Alan Cox, cernekee,
Linux Kernel
On Sun, Mar 6, 2016 at 12:11 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
...
>
> How about you introduce a reasonable max size for a transaction instead
> (256K?), and compare data_size with that? Might want to check with the EC folks
> what they expect larges transactions to be from their side, and go with
> a margin above that.
Make sense, patch coming.
>
>
> Also, in your commit message you should refer to the CVE this fixes.
AFAIK, no CVE for this bug.
>
>
> -Olof
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] platform/chrome: cros_ec_dev - Fix security issue
2016-03-08 17:02 ` Gwendal Grignou
@ 2016-03-08 17:13 ` Gwendal Grignou
2016-05-11 17:58 ` Olof Johansson
0 siblings, 1 reply; 8+ messages in thread
From: Gwendal Grignou @ 2016-03-08 17:13 UTC (permalink / raw)
To: rdunlap, olofj, alan, cernekee; +Cc: linux-kernel
Prevent memory scribble by checking that ioctl buffer size parameters
are sane.
Without this check, on 32 bits system, if .insize = 0xffffffff - 20 and
.outsize the amount to scribble, we would overflow, allocate a small
amounts and be able to write outside of the malloc'ed area.
Adding a hard limit allows argument checking of the ioctl. With the
current EC, it is expected .insize and .outsize to be at around 512 bytes
or less.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/platform/chrome/cros_ec_dev.c | 4 ++++
drivers/platform/chrome/cros_ec_proto.c | 4 ++--
include/linux/mfd/cros_ec.h | 6 ++++--
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index d45cd25..187470c 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -137,6 +137,10 @@ static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
return -EFAULT;
+ if ((u_cmd.outsize > EC_MAX_MSG_BYTES) ||
+ (u_cmd.insize > EC_MAX_MSG_BYTES))
+ return -EINVAL;
+
s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
GFP_KERNEL);
if (!s_cmd)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 990308c..b6e161f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -298,8 +298,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
ec_dev->max_passthru = 0;
ec_dev->pkt_xfer = NULL;
- ec_dev->din_size = EC_MSG_BYTES;
- ec_dev->dout_size = EC_MSG_BYTES;
+ ec_dev->din_size = EC_PROTO2_MSG_BYTES;
+ ec_dev->dout_size = EC_PROTO2_MSG_BYTES;
} else {
/*
* It's possible for a test to occur too early when
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 494682c..9cb77ed 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -50,9 +50,11 @@ enum {
EC_MSG_TX_TRAILER_BYTES,
EC_MSG_RX_PROTO_BYTES = 3,
- /* Max length of messages */
- EC_MSG_BYTES = EC_PROTO2_MAX_PARAM_SIZE +
+ /* Max length of messages for proto 2*/
+ EC_PROTO2_MSG_BYTES = EC_PROTO2_MAX_PARAM_SIZE +
EC_MSG_TX_PROTO_BYTES,
+
+ EC_MAX_MSG_BYTES = 64 * 1024,
};
/*
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] platform/chrome: cros_ec_dev - Fix security issue
2016-03-08 17:13 ` [PATCH v3] " Gwendal Grignou
@ 2016-05-11 17:58 ` Olof Johansson
0 siblings, 0 replies; 8+ messages in thread
From: Olof Johansson @ 2016-05-11 17:58 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: rdunlap, olofj, alan, cernekee, linux-kernel
On Tue, Mar 08, 2016 at 09:13:52AM -0800, Gwendal Grignou wrote:
> Prevent memory scribble by checking that ioctl buffer size parameters
> are sane.
> Without this check, on 32 bits system, if .insize = 0xffffffff - 20 and
> .outsize the amount to scribble, we would overflow, allocate a small
> amounts and be able to write outside of the malloc'ed area.
> Adding a hard limit allows argument checking of the ioctl. With the
> current EC, it is expected .insize and .outsize to be at around 512 bytes
> or less.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Applied now.
-Olof
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-05-11 19:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-01 20:33 Security hole in cros_ec_dev.c on 32bit chrome hosts Alan Cox
2016-03-03 5:58 ` [PATCH] platform/chrome: cros_ec_dev - Fix security issue Gwendal Grignou
2016-03-03 18:35 ` Randy Dunlap
2016-03-03 19:00 ` [PATCH v2] " Gwendal Grignou
2016-03-06 20:11 ` Olof Johansson
2016-03-08 17:02 ` Gwendal Grignou
2016-03-08 17:13 ` [PATCH v3] " Gwendal Grignou
2016-05-11 17:58 ` Olof Johansson
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).