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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Sun, Mar 6, 2016 at 12:11 PM, Olof Johansson <[email protected]> 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
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 <[email protected]>
---
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