[PATCH v1] HID: make arrays usage and value to be the same

[PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

The HID subsystem allows an "HID report field" to have a different
number of "values" and "usages" when it is allocated. When a field
struct is created, the size of the usage array is guaranteed to be at
least as large as the values array, but it may be larger. This leads to
a potential out-of-bounds write in
__hidinput_change_resolution_multipliers() and an out-of-bounds read in
hidinput_count_leds().

To fix this, let's make sure that both the usage and value arrays are
the same size.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/hid/hid-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 56172fe6995c..8a8b2b982f83 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
  * Register a new field for this report.
  */
 
-static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
+static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
 {
 	struct hid_field *field;
 
@@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 
 	field = kzalloc((sizeof(struct hid_field) +
 			 usages * sizeof(struct hid_usage) +
-			 values * sizeof(unsigned)), GFP_KERNEL);
+			 usages * sizeof(unsigned)), GFP_KERNEL);
 	if (!field)
 		return NULL;
 
@@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 	usages = max_t(unsigned, parser->local.usage_index,
 				 parser->global.report_count);
 
-	field = hid_register_field(report, usages, parser->global.report_count);
+	field = hid_register_field(report, usages);
 	if (!field)
 		return 0;
 
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Greg KH]

On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> The HID subsystem allows an "HID report field" to have a different
> number of "values" and "usages" when it is allocated. When a field
> struct is created, the size of the usage array is guaranteed to be at
> least as large as the values array, but it may be larger. This leads to
> a potential out-of-bounds write in
> __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> hidinput_count_leds().
> 
> To fix this, let's make sure that both the usage and value arrays are
> the same size.
> 
> Signed-off-by: Will McVicker <willmcvicker@google.com>

Any reason not to also add a cc: stable on this?

And, has this always been the case, or was this caused by some specific
commit in the past?  If so, a "Fixes:" tag is always nice to included.

And finally, as you have a fix for this already, no need to cc:
security@k.o as there's nothing the people there can do about it now :)

thanks,

greg k-h

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

On Sat, Dec 05, 2020 at 09:59:57AM +0100, Greg KH wrote:
> On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> > The HID subsystem allows an "HID report field" to have a different
> > number of "values" and "usages" when it is allocated. When a field
> > struct is created, the size of the usage array is guaranteed to be at
> > least as large as the values array, but it may be larger. This leads to
> > a potential out-of-bounds write in
> > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > hidinput_count_leds().
> > 
> > To fix this, let's make sure that both the usage and value arrays are
> > the same size.
> > 
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> 
> Any reason not to also add a cc: stable on this?
No reason not to include stable. CC'd here.

> 
> And, has this always been the case, or was this caused by some specific
> commit in the past?  If so, a "Fixes:" tag is always nice to included.
I dug into the history and it's been like this for the past 10 years. So yeah
pretty much always like this.

> 
> And finally, as you have a fix for this already, no need to cc:
> security@k.o as there's nothing the people there can do about it now :)
Is that short for security@kernel.org? If yes, then I did include them. If no,
do you mind explaining?

> 
> thanks,
> 
> greg k-h

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Greg KH]

On Mon, Dec 07, 2020 at 09:55:48AM -0800, Will McVicker wrote:
> On Sat, Dec 05, 2020 at 09:59:57AM +0100, Greg KH wrote:
> > On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> > > The HID subsystem allows an "HID report field" to have a different
> > > number of "values" and "usages" when it is allocated. When a field
> > > struct is created, the size of the usage array is guaranteed to be at
> > > least as large as the values array, but it may be larger. This leads to
> > > a potential out-of-bounds write in
> > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > hidinput_count_leds().
> > > 
> > > To fix this, let's make sure that both the usage and value arrays are
> > > the same size.
> > > 
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > 
> > Any reason not to also add a cc: stable on this?
> No reason not to include stable. CC'd here.
> 
> > 
> > And, has this always been the case, or was this caused by some specific
> > commit in the past?  If so, a "Fixes:" tag is always nice to included.
> I dug into the history and it's been like this for the past 10 years. So yeah
> pretty much always like this.
> 
> > 
> > And finally, as you have a fix for this already, no need to cc:
> > security@k.o as there's nothing the people there can do about it now :)
> Is that short for security@kernel.org? If yes, then I did include them. If no,
> do you mind explaining?

Yes, I see you included it, my point was that once you have a patch,
there is no need to include this email address as all we do at this
address is work to match up a problem with a developer that can create a
fix.  You already did this, so no need for us to get involved at all! :)

thanks,

greg k-h

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

On Mon, Dec 07, 2020 at 07:24:16PM +0100, Greg KH wrote:
> On Mon, Dec 07, 2020 at 09:55:48AM -0800, Will McVicker wrote:
> > On Sat, Dec 05, 2020 at 09:59:57AM +0100, Greg KH wrote:
> > > On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> > > > The HID subsystem allows an "HID report field" to have a different
> > > > number of "values" and "usages" when it is allocated. When a field
> > > > struct is created, the size of the usage array is guaranteed to be at
> > > > least as large as the values array, but it may be larger. This leads to
> > > > a potential out-of-bounds write in
> > > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > > hidinput_count_leds().
> > > > 
> > > > To fix this, let's make sure that both the usage and value arrays are
> > > > the same size.
> > > > 
> > > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > 
> > > Any reason not to also add a cc: stable on this?
> > No reason not to include stable. CC'd here.
> > 
> > > 
> > > And, has this always been the case, or was this caused by some specific
> > > commit in the past?  If so, a "Fixes:" tag is always nice to included.
> > I dug into the history and it's been like this for the past 10 years. So yeah
> > pretty much always like this.
> > 
> > > 
> > > And finally, as you have a fix for this already, no need to cc:
> > > security@k.o as there's nothing the people there can do about it now :)
> > Is that short for security@kernel.org? If yes, then I did include them. If no,
> > do you mind explaining?
> 
> Yes, I see you included it, my point was that once you have a patch,
> there is no need to include this email address as all we do at this
> address is work to match up a problem with a developer that can create a
> fix.  You already did this, so no need for us to get involved at all! :)
> 
> thanks,
> 
> greg k-h
Ah okay, thanks for the explanation!

--Will

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> The HID subsystem allows an "HID report field" to have a different
> number of "values" and "usages" when it is allocated. When a field
> struct is created, the size of the usage array is guaranteed to be at
> least as large as the values array, but it may be larger. This leads to
> a potential out-of-bounds write in
> __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> hidinput_count_leds().
> 
> To fix this, let's make sure that both the usage and value arrays are
> the same size.
> 
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  drivers/hid/hid-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 56172fe6995c..8a8b2b982f83 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
>   * Register a new field for this report.
>   */
>  
> -static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
> +static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
>  {
>  	struct hid_field *field;
>  
> @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
>  
>  	field = kzalloc((sizeof(struct hid_field) +
>  			 usages * sizeof(struct hid_usage) +
> -			 values * sizeof(unsigned)), GFP_KERNEL);
> +			 usages * sizeof(unsigned)), GFP_KERNEL);
>  	if (!field)
>  		return NULL;
>  
> @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
>  	usages = max_t(unsigned, parser->local.usage_index,
>  				 parser->global.report_count);
>  
> -	field = hid_register_field(report, usages, parser->global.report_count);
> +	field = hid_register_field(report, usages);
>  	if (!field)
>  		return 0;
>  
> -- 
> 2.29.2.576.ga3fc446d84-goog
> 

Hi Jiri and Benjamin,

This is a friendly reminder in case this got lost in your inbox.

Thanks,
Will

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Jiri Kosina]

On Mon, 14 Dec 2020, Will McVicker wrote:

> > The HID subsystem allows an "HID report field" to have a different
> > number of "values" and "usages" when it is allocated. When a field
> > struct is created, the size of the usage array is guaranteed to be at
> > least as large as the values array, but it may be larger. This leads to
> > a potential out-of-bounds write in
> > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > hidinput_count_leds().
> > 
> > To fix this, let's make sure that both the usage and value arrays are
> > the same size.
> > 
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  drivers/hid/hid-core.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 56172fe6995c..8a8b2b982f83 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
> >   * Register a new field for this report.
> >   */
> >  
> > -static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
> > +static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
> >  {
> >  	struct hid_field *field;
> >  
> > @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
> >  
> >  	field = kzalloc((sizeof(struct hid_field) +
> >  			 usages * sizeof(struct hid_usage) +
> > -			 values * sizeof(unsigned)), GFP_KERNEL);
> > +			 usages * sizeof(unsigned)), GFP_KERNEL);
> >  	if (!field)
> >  		return NULL;
> >  
> > @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
> >  	usages = max_t(unsigned, parser->local.usage_index,
> >  				 parser->global.report_count);
> >  
> > -	field = hid_register_field(report, usages, parser->global.report_count);
> > +	field = hid_register_field(report, usages);
> >  	if (!field)
> >  		return 0;
> >  
> > -- 
> > 2.29.2.576.ga3fc446d84-goog
> > 
> 
> Hi Jiri and Benjamin,
> 
> This is a friendly reminder in case this got lost in your inbox.

Hi Will,

I am planning to merge it once the merge window is over.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

Great! Thanks for the reply.

--Will

On Thu, Dec 17, 2020 at 2:19 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 14 Dec 2020, Will McVicker wrote:
>
> > > The HID subsystem allows an "HID report field" to have a different
> > > number of "values" and "usages" when it is allocated. When a field
> > > struct is created, the size of the usage array is guaranteed to be at
> > > least as large as the values array, but it may be larger. This leads to
> > > a potential out-of-bounds write in
> > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > hidinput_count_leds().
> > >
> > > To fix this, let's make sure that both the usage and value arrays are
> > > the same size.
> > >
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > ---
> > >  drivers/hid/hid-core.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 56172fe6995c..8a8b2b982f83 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
> > >   * Register a new field for this report.
> > >   */
> > >
> > > -static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
> > > +static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
> > >  {
> > >     struct hid_field *field;
> > >
> > > @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
> > >
> > >     field = kzalloc((sizeof(struct hid_field) +
> > >                      usages * sizeof(struct hid_usage) +
> > > -                    values * sizeof(unsigned)), GFP_KERNEL);
> > > +                    usages * sizeof(unsigned)), GFP_KERNEL);
> > >     if (!field)
> > >             return NULL;
> > >
> > > @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
> > >     usages = max_t(unsigned, parser->local.usage_index,
> > >                              parser->global.report_count);
> > >
> > > -   field = hid_register_field(report, usages, parser->global.report_count);
> > > +   field = hid_register_field(report, usages);
> > >     if (!field)
> > >             return 0;
> > >
> > > --
> > > 2.29.2.576.ga3fc446d84-goog
> > >
> >
> > Hi Jiri and Benjamin,
> >
> > This is a friendly reminder in case this got lost in your inbox.
>
> Hi Will,
>
> I am planning to merge it once the merge window is over.
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Will McVicker]

Hi Jiri,

I noticed this hasn't merged yet. So just sending a friendly reminder.

Thanks,
Will

On Thu, Dec 17, 2020 at 10:42 AM Will McVicker <willmcvicker@google.com> wrote:
>
> Great! Thanks for the reply.
>
> --Will
>
> On Thu, Dec 17, 2020 at 2:19 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Mon, 14 Dec 2020, Will McVicker wrote:
> >
> > > > The HID subsystem allows an "HID report field" to have a different
> > > > number of "values" and "usages" when it is allocated. When a field
> > > > struct is created, the size of the usage array is guaranteed to be at
> > > > least as large as the values array, but it may be larger. This leads to
> > > > a potential out-of-bounds write in
> > > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > > hidinput_count_leds().
> > > >
> > > > To fix this, let's make sure that both the usage and value arrays are
> > > > the same size.
> > > >
> > > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > > ---
> > > >  drivers/hid/hid-core.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 56172fe6995c..8a8b2b982f83 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
> > > >   * Register a new field for this report.
> > > >   */
> > > >
> > > > -static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
> > > > +static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
> > > >  {
> > > >     struct hid_field *field;
> > > >
> > > > @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
> > > >
> > > >     field = kzalloc((sizeof(struct hid_field) +
> > > >                      usages * sizeof(struct hid_usage) +
> > > > -                    values * sizeof(unsigned)), GFP_KERNEL);
> > > > +                    usages * sizeof(unsigned)), GFP_KERNEL);
> > > >     if (!field)
> > > >             return NULL;
> > > >
> > > > @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
> > > >     usages = max_t(unsigned, parser->local.usage_index,
> > > >                              parser->global.report_count);
> > > >
> > > > -   field = hid_register_field(report, usages, parser->global.report_count);
> > > > +   field = hid_register_field(report, usages);
> > > >     if (!field)
> > > >             return 0;
> > > >
> > > > --
> > > > 2.29.2.576.ga3fc446d84-goog
> > > >
> > >
> > > Hi Jiri and Benjamin,
> > >
> > > This is a friendly reminder in case this got lost in your inbox.
> >
> > Hi Will,
> >
> > I am planning to merge it once the merge window is over.
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >

Re: [PATCH v1] HID: make arrays usage and value to be the same

[Jiri Kosina]

On Sat, 5 Dec 2020, Will McVicker wrote:

> The HID subsystem allows an "HID report field" to have a different
> number of "values" and "usages" when it is allocated. When a field
> struct is created, the size of the usage array is guaranteed to be at
> least as large as the values array, but it may be larger. This leads to
> a potential out-of-bounds write in
> __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> hidinput_count_leds().
> 
> To fix this, let's make sure that both the usage and value arrays are
> the same size.

Now applied, sorry for the delay and thanks for the fix.

-- 
Jiri Kosina
SUSE Labs