Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1351209 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue Olof Johansson <olof@lixom.net> - 2016-03-06 23:20 +0100
    Re: [PATCH v2] platform/chrome: cros_ec_dev - Fix security issue Gwendal Grignou <gwendal@chromium.org> - 2016-03-08 18:10 +0100
      [PATCH v3] platform/chrome: cros_ec_dev - Fix security issue Gwendal Grignou <gwendal@chromium.org> - 2016-03-08 18:20 +0100

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

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

[toc] | [next] | [standalone]

#1353243

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web