On Mon, Sep 9, 2013 at 3:02 PM, Benjamin Tissoires <[email protected]> wrote: > On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <[email protected]> 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 <[email protected]> >> Cc: [email protected] >> --- >> 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 <[email protected]> Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
