Message ID | alpine.LNX.2.00.1308282223090.22181@pobox.suse.cz (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Jiri Kosina |
Headers | show |
On Wed, 28 Aug 2013, Jiri Kosina wrote: > From: Kees Cook <keescook@chromium.org> > > Defensively check that the field to be worked on is not NULL. > > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: stable@kernel.org > --- > drivers/hid/hid-core.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 55798b2..192be6b 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report); > > int hid_set_field(struct hid_field *field, unsigned offset, __s32 value) > { > - unsigned size = field->report_size; > + unsigned size; > + > + if (!field) > + return -1; > + > + size = field->report_size; Kees, do you actually see any way how field could ever be null in hid_set_field()?
On Wed, Aug 28, 2013 at 1:32 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Wed, 28 Aug 2013, Jiri Kosina wrote: > >> From: Kees Cook <keescook@chromium.org> >> >> Defensively check that the field to be worked on is not NULL. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: stable@kernel.org >> --- >> drivers/hid/hid-core.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index 55798b2..192be6b 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report); >> >> int hid_set_field(struct hid_field *field, unsigned offset, __s32 value) >> { >> - unsigned size = field->report_size; >> + unsigned size; >> + >> + if (!field) >> + return -1; >> + >> + size = field->report_size; > > Kees, > > do you actually see any way how field could ever be null in > hid_set_field()? Yes, report->field[0] may be uninitialized (NULL due to kzalloc) if hid_add_field() bails out during the padding field check: if (!parser->local.usage_index) /* Ignore padding fields */ return 0; The report will be registered (earlier call to hid_register_report() was successful), but the entire field list will be NULL. The hid-ntrig.c fix is a fix for such a problem (report->field[0]->value[0]) where field[0] can be NULL. I was able to make a reproduction case for this, with the crash can be seen in the hid-ntrig.c patch. -Kees
On Wed, 28 Aug 2013, Jiri Kosina wrote: > From: Kees Cook <keescook@chromium.org> > > Defensively check that the field to be worked on is not NULL. Applied. > > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: stable@kernel.org > --- > drivers/hid/hid-core.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 55798b2..192be6b 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report); > > int hid_set_field(struct hid_field *field, unsigned offset, __s32 value) > { > - unsigned size = field->report_size; > + unsigned size; > + > + if (!field) > + return -1; > + > + size = field->report_size; > > hid_dump_input(field->report->device, field->usage + offset, value); > > -- > Jiri Kosina > SUSE Labs >
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 55798b2..192be6b 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report); int hid_set_field(struct hid_field *field, unsigned offset, __s32 value) { - unsigned size = field->report_size; + unsigned size; + + if (!field) + return -1; + + size = field->report_size; hid_dump_input(field->report->device, field->usage + offset, value);