| Message ID | 20130305130626.GA17801@longonot.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Dan, On Tue, Mar 5, 2013 at 2:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > We can't trust dj_report->device_index because it comes from the > user and hasn't been range checked. It is used as an offset into > the djrcv_dev->paired_dj_devices[] array which has 7 elements. > There is one range check already in logi_dj_recv_add_djhid_device() > and I have copy and pasted that here. > > DJ_DEVICE_INDEX_MIN is 1. > DJ_DEVICE_INDEX_MAX is 6. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > This is a static checker fix and I'm not certain it's correct, > please look it over carefully. > > I do not know this code well, so I don't know why a zero index is > invalid. > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 9500f2f..c01cd25 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -701,6 +701,13 @@ static int logi_dj_raw_event(struct hid_device *hdev, > * anything else with it. > */ > > + if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) || > + (dj_report->device_index > DJ_DEVICE_INDEX_MAX)) { > + dev_err(&hdev->dev, "%s: invalid device index:%d\n", __func__, > + dj_report->device_index); > + return true; > + } > + Sorry, but it's a NACK in its current form: - device_index == 0 is the receiver. IIRC, the receiver sends the different REPORT_TYPE_NOTIF_*. So I would say that this patch blocks events from the receiver. - the DJ protocol specifies that the device index is between 1 and 6 and that 0 means the receiver. So unless there is a problem in the USB line from the receiver to the driver, device index will be between 1 and 6 for input events. This is only true as long as with stay with real firmware from Logitech, not from events from uhid. If you want to add a check, it need to be in logi_dj_recv_forward_report(). The current access to djrcv_dev->paired_dj_devices[] in delayedwork_callback() has been removed in latest HID tree: https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/log/?h=for-3.10/logitech Cheers, Benjamin > spin_lock_irqsave(&djrcv_dev->lock, flags); > if (dj_report->report_id == REPORT_ID_DJ_SHORT) { > switch (dj_report->report_type) { > -- > 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
Hi Dan, On Tue, Mar 5, 2013 at 3:47 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > Hi Dan, > > On Tue, Mar 5, 2013 at 2:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> We can't trust dj_report->device_index because it comes from the >> user and hasn't been range checked. It is used as an offset into >> the djrcv_dev->paired_dj_devices[] array which has 7 elements. >> There is one range check already in logi_dj_recv_add_djhid_device() >> and I have copy and pasted that here. >> >> DJ_DEVICE_INDEX_MIN is 1. >> DJ_DEVICE_INDEX_MAX is 6. >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> This is a static checker fix and I'm not certain it's correct, >> please look it over carefully. >> >> I do not know this code well, so I don't know why a zero index is >> invalid. >> >> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c >> index 9500f2f..c01cd25 100644 >> --- a/drivers/hid/hid-logitech-dj.c >> +++ b/drivers/hid/hid-logitech-dj.c >> @@ -701,6 +701,13 @@ static int logi_dj_raw_event(struct hid_device *hdev, >> * anything else with it. >> */ >> >> + if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) || >> + (dj_report->device_index > DJ_DEVICE_INDEX_MAX)) { >> + dev_err(&hdev->dev, "%s: invalid device index:%d\n", __func__, >> + dj_report->device_index); >> + return true; >> + } >> + > > Sorry, but it's a NACK in its current form: > - device_index == 0 is the receiver. IIRC, the receiver sends the > different REPORT_TYPE_NOTIF_*. So I would say that this patch blocks > events from the receiver. Correct. This would block notifications from the receiver. > - the DJ protocol specifies that the device index is between 1 and 6 > and that 0 means the receiver. So unless there is a problem in the USB > line from the receiver to the driver, device index will be between 1 > and 6 for input events. This is only true as long as with stay with > real firmware from Logitech, not from events from uhid. > > If you want to add a check, it need to be in logi_dj_recv_forward_report(). > The current access to djrcv_dev->paired_dj_devices[] in > delayedwork_callback() has been removed in latest HID tree: > https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/log/?h=for-3.10/logitech The test could be added there. It would be quite defensive though, as we should always get the right device indexes. > > Cheers, > Benjamin > >> spin_lock_irqsave(&djrcv_dev->lock, flags); >> if (dj_report->report_id == REPORT_ID_DJ_SHORT) { >> switch (dj_report->report_type) { >> -- >> 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 -- 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 Tue, Mar 05, 2013 at 05:34:08PM +0100, Nestor Lopez Casado wrote: > > If you want to add a check, it need to be in logi_dj_recv_forward_report(). > > The current access to djrcv_dev->paired_dj_devices[] in > > delayedwork_callback() has been removed in latest HID tree: > > https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/log/?h=for-3.10/logitech > The test could be added there. It would be quite defensive though, as > we should always get the right device indexes. Ah... I appologize for the noise. I misunderstood how hid_input_report() works. I thought it could be called with user supplied parameters. Never mind. regards, dan carpenter -- 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-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 9500f2f..c01cd25 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -701,6 +701,13 @@ static int logi_dj_raw_event(struct hid_device *hdev, * anything else with it. */ + if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) || + (dj_report->device_index > DJ_DEVICE_INDEX_MAX)) { + dev_err(&hdev->dev, "%s: invalid device index:%d\n", __func__, + dj_report->device_index); + return true; + } + spin_lock_irqsave(&djrcv_dev->lock, flags); if (dj_report->report_id == REPORT_ID_DJ_SHORT) { switch (dj_report->report_type) {
We can't trust dj_report->device_index because it comes from the user and hasn't been range checked. It is used as an offset into the djrcv_dev->paired_dj_devices[] array which has 7 elements. There is one range check already in logi_dj_recv_add_djhid_device() and I have copy and pasted that here. DJ_DEVICE_INDEX_MIN is 1. DJ_DEVICE_INDEX_MAX is 6. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- This is a static checker fix and I'm not certain it's correct, please look it over carefully. I do not know this code well, so I don't know why a zero index is invalid. -- 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