Security hole in cros_ec_dev.c on 32bit chrome hosts

Security hole in cros_ec_dev.c on 32bit chrome hosts

[Alan Cox]

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

[PATCH] platform/chrome: cros_ec_dev - Fix security issue

[Gwendal Grignou]

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

Re: [PATCH] platform/chrome: cros_ec_dev - Fix security issue

[Randy Dunlap]

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

[PATCH v2] platform/chrome: cros_ec_dev - Fix security issue

[Gwendal Grignou]

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

Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue

[Olof Johansson]

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

Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue

[Gwendal Grignou]

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

[PATCH v3] platform/chrome: cros_ec_dev - Fix security issue

[Gwendal Grignou]

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

Re: [PATCH v3] platform/chrome: cros_ec_dev - Fix security issue

[Olof Johansson]

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