[PATCH 13/14] HID: picolcd_core: validate output report details

[PATCH 13/14] HID: picolcd_core: validate output report details

[Jiri Kosina]

From: Kees Cook <keescook@chromium.org>

A HID device could send a malicious output report that would cause the
picolcd HID driver to trigger a NULL dereference during attr file writing.

CVE-2013-2899

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-picolcd_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index b48092d..72bba1e 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
 		buf += 10;
 		cnt -= 10;
 	}
-	if (!report)
+	if (!report || report->maxfield < 1)
 		return -EINVAL;
 
 	while (cnt > 0 && (buf[cnt-1] == '\n' || buf[cnt-1] == '\r'))
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 13/14] HID: picolcd_core: validate output report details

[Bruno Prémont]

On Wed, 28 August 2013 Jiri Kosina <jkosina@suse.cz> wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> A HID device could send a malicious output report that would cause the
> picolcd HID driver to trigger a NULL dereference during attr file writing.
> 
> CVE-2013-2899
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
>  drivers/hid/hid-picolcd_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index b48092d..72bba1e 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
>  		buf += 10;
>  		cnt -= 10;
>  	}
> -	if (!report)
> +	if (!report || report->maxfield < 1)
>  		return -EINVAL;
>  
>  	while (cnt > 0 && (buf[cnt-1] == '\n' || buf[cnt-1] == '\r'))

I will check tomorrow or Friday evening what the documentation I have
says for this report and test, might be a report->maxfield != 1 would
be even better suited.

Too late today for looking into it.

Bruno

Re: [PATCH 13/14] HID: picolcd_core: validate output report details

[Bruno Prémont]

Hi Kees, Jiri,

On Wed, 28 August 2013 Jiri Kosina <jkosina@suse.cz> wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> A HID device could send a malicious output report that would cause the
> picolcd HID driver to trigger a NULL dereference during attr file writing.
> 
> CVE-2013-2899
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
>  drivers/hid/hid-picolcd_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index b48092d..72bba1e 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
>  		buf += 10;
>  		cnt -= 10;
>  	}
> -	if (!report)
> +	if (!report || report->maxfield < 1)

Please do
  +     if (!report || report->maxfield != 1)

That way we are consistent with whole picolcd driver and a device
deciding to change its HID-ABI will be properly detected.

With that adjustment, Acked-by/Reviewed-by me

Thanks,
Bruno

>  		return -EINVAL;
>  
>  	while (cnt > 0 && (buf[cnt-1] == '\n' || buf[cnt-1] == '\r'))

Re: [PATCH 13/14] HID: picolcd_core: validate output report details

[Jiri Kosina]

On Sat, 31 Aug 2013, Bruno Prémont wrote:

> Hi Kees, Jiri,
> 
> On Wed, 28 August 2013 Jiri Kosina <jkosina@suse.cz> wrote:
> > From: Kees Cook <keescook@chromium.org>
> > 
> > A HID device could send a malicious output report that would cause the
> > picolcd HID driver to trigger a NULL dereference during attr file writing.
> > 
> > CVE-2013-2899
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Cc: stable@kernel.org
> > ---
> >  drivers/hid/hid-picolcd_core.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> > index b48092d..72bba1e 100644
> > --- a/drivers/hid/hid-picolcd_core.c
> > +++ b/drivers/hid/hid-picolcd_core.c
> > @@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
> >  		buf += 10;
> >  		cnt -= 10;
> >  	}
> > -	if (!report)
> > +	if (!report || report->maxfield < 1)
> 
> Please do
>   +     if (!report || report->maxfield != 1)
> 
> That way we are consistent with whole picolcd driver and a device
> deciding to change its HID-ABI will be properly detected.
> 
> With that adjustment, Acked-by/Reviewed-by me

Applied with that adjustment. Thanks,

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html