Message ID | 1378312645-27736-5-git-send-email-keescook@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Jiri Kosina |
Headers | show |
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote: > A HID device could send a malicious output report that would cause the > steelseries HID driver to write beyond the output report allocation > during initialization, causing a heap overflow: > > [ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410 > ... > [ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten > > CVE-2013-2891 > > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: stable@kernel.org > --- > drivers/hid/hid-steelseries.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c > index d164911..29f328f 100644 > --- a/drivers/hid/hid-steelseries.c > +++ b/drivers/hid/hid-steelseries.c > @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev, > goto err_free; > } > > + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) { > + ret = -ENODEV; > + goto err_free; There is a problem here in case of a failure: hid_parse() allocates a lot of memory and sets the flag HID_STAT_PARSED, but we are leaving the probe() without clearing all this stuff. In hid_parse(), when a failure is detected, we call hid_close_report(), so we also should close the report in the same way here (but hid_close_report() is a static function). Cheers, Benjamin > + } > + > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > if (ret) { > hid_err(hdev, "hw start failed\n"); > -- > 1.7.9.5 > > -- > 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 -- 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
On Mon, Sep 9, 2013 at 3:02 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote: >> A HID device could send a malicious output report that would cause the >> steelseries HID driver to write beyond the output report allocation >> during initialization, causing a heap overflow: >> >> [ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410 >> ... >> [ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten >> >> CVE-2013-2891 >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: stable@kernel.org >> --- >> drivers/hid/hid-steelseries.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c >> index d164911..29f328f 100644 >> --- a/drivers/hid/hid-steelseries.c >> +++ b/drivers/hid/hid-steelseries.c >> @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev, >> goto err_free; >> } >> >> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) { >> + ret = -ENODEV; >> + goto err_free; > > There is a problem here in case of a failure: > hid_parse() allocates a lot of memory and sets the flag > HID_STAT_PARSED, but we are leaving the probe() without clearing all > this stuff. > In hid_parse(), when a failure is detected, we call > hid_close_report(), so we also should close the report in the same way > here (but hid_close_report() is a static function). > Sorry, my bad. hid_close_report() is called upon ->probe() failure. So this is perfectly good. Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin -- 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
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c index d164911..29f328f 100644 --- a/drivers/hid/hid-steelseries.c +++ b/drivers/hid/hid-steelseries.c @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev, goto err_free; } + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) { + ret = -ENODEV; + goto err_free; + } + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); if (ret) { hid_err(hdev, "hw start failed\n");
A HID device could send a malicious output report that would cause the steelseries HID driver to write beyond the output report allocation during initialization, causing a heap overflow: [ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410 ... [ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten CVE-2013-2891 Signed-off-by: Kees Cook <keescook@chromium.org> Cc: stable@kernel.org --- drivers/hid/hid-steelseries.c | 5 +++++ 1 file changed, 5 insertions(+)