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

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

[Gwendal Grignou]

On Thu, Mar 18, 2021 at 1:20 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 18 Mar 2021, Greg KH wrote:
>
> > On Thu, Mar 18, 2021 at 07:54:43AM +0000, Lee Jones wrote:
> > > On Wed, 17 Mar 2021, Gwendal Grignou wrote:
> > >
> > > > commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.
> > > >
> > > > 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: Olof Johansson <olof@lixom.net>
> > > > 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 2b331d5b9e799..e16d82bb36a9d 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 5c285f2b3a650..d20190c8f0c06 100644
> > > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > > @@ -311,8 +311,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 3ab3cede28eac..93c14e9df6309 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,
> > >
> > > Nit: Better to not tab the '=' so far and place it all on one line.
> > >
> > > Checkpatch now only complains about lines exceeding 100 chars.
> > >
> > > Once fixed, feel free to apply my:
> > >
> > >   Acked-by: Lee Jones <lee.jones@linaro.org>
> >
> > This commit is already in 4.7, and is from 2016, so I don't know why you
> > are reviewing it :)
My mistake, I overlooked the branch information is removed when
formatting a patch: it is for branch linux-4.4.y in linux stable git
tree.

>
> Heh!  It looked like a standard patch at first glance.
>
> Must have skipped over the "commit" line in the commit log.
>
> I wonder why it doesn't have my Ack on it already then?
You were not in the loop on the original thread:
https://lkml.org/lkml/2016/3/8/628
Sorry about that,

Gwendal.


>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Thu, 18 Mar 2021, Greg KH wrote:

> On Thu, Mar 18, 2021 at 07:54:43AM +0000, Lee Jones wrote:
> > On Wed, 17 Mar 2021, Gwendal Grignou wrote:
> > 
> > > commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.
> > > 
> > > 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: Olof Johansson <olof@lixom.net>
> > > 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 2b331d5b9e799..e16d82bb36a9d 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 5c285f2b3a650..d20190c8f0c06 100644
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -311,8 +311,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 3ab3cede28eac..93c14e9df6309 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,
> > 
> > Nit: Better to not tab the '=' so far and place it all on one line.
> > 
> > Checkpatch now only complains about lines exceeding 100 chars.
> > 
> > Once fixed, feel free to apply my:
> > 
> >   Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> This commit is already in 4.7, and is from 2016, so I don't know why you
> are reviewing it :)

Heh!  It looked like a standard patch at first glance.

Must have skipped over the "commit" line in the commit log.

I wonder why it doesn't have my Ack on it already then?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Greg KH]

On Thu, Mar 18, 2021 at 07:54:43AM +0000, Lee Jones wrote:
> On Wed, 17 Mar 2021, Gwendal Grignou wrote:
> 
> > commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.
> > 
> > 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: Olof Johansson <olof@lixom.net>
> > 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 2b331d5b9e799..e16d82bb36a9d 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 5c285f2b3a650..d20190c8f0c06 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -311,8 +311,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 3ab3cede28eac..93c14e9df6309 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,
> 
> Nit: Better to not tab the '=' so far and place it all on one line.
> 
> Checkpatch now only complains about lines exceeding 100 chars.
> 
> Once fixed, feel free to apply my:
> 
>   Acked-by: Lee Jones <lee.jones@linaro.org>

This commit is already in 4.7, and is from 2016, so I don't know why you
are reviewing it :)

thanks,

greg k-h

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

[Lee Jones]

On Wed, 17 Mar 2021, Gwendal Grignou wrote:

> commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.
> 
> 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: Olof Johansson <olof@lixom.net>
> 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 2b331d5b9e799..e16d82bb36a9d 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 5c285f2b3a650..d20190c8f0c06 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -311,8 +311,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 3ab3cede28eac..93c14e9df6309 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,

Nit: Better to not tab the '=' so far and place it all on one line.

Checkpatch now only complains about lines exceeding 100 chars.

Once fixed, feel free to apply my:

  Acked-by: Lee Jones <lee.jones@linaro.org>

> +
> +	EC_MAX_MSG_BYTES		= 64 * 1024,
>  };
>  
>  /*

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Greg KH]

On Wed, Mar 17, 2021 at 04:55:22PM -0700, Gwendal Grignou wrote:
> commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.
> 
> 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: Olof Johansson <olof@lixom.net>
> 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(-)

What stable tree(s) are you wanting this to be applied to?

Always give us a hint...

thanks,

greg k-h

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

[Gwendal Grignou]

commit 5d749d0bbe811c10d9048cde6dfebc761713abfd upstream.

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: Olof Johansson <olof@lixom.net>
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 2b331d5b9e799..e16d82bb36a9d 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 5c285f2b3a650..d20190c8f0c06 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -311,8 +311,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 3ab3cede28eac..93c14e9df6309 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.31.0.rc2.261.g7f71774620-goog