| Message ID | 20201205004848.2541215-1-willmcvicker@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | ed9be64eefe26d7d8b0b5b9fa3ffdf425d87a01f |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v1] HID: make arrays usage and value to be the same | expand |
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
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
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
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
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
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.
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 >
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 > >
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.
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;
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(-)