Hi Jiri,
Here are four patches intended for the 3.19 stream and are based on
jikos/hid (for-next, v3.18-rc4-144-gd9372ee).
* The first is actually from Benjamin Tissoires, but modified to remove
a now unneeded goto.
* The second one depends on the first (it could work without, but there
will be a context mismatch).
* The third one can be applied independently of the others and is needed
to avoid a possible buffer overread.
* The fourth and final patch fixes an unbalanced hid_device_io_start().
Tested by booting with three paired USB devices (QEMU + USB
passthrough), two of them are powered off and one M525 is active. evbug
registers mouse events.
Kind regards,
Peter
Peter Wu (4):
HID: logitech-hidpp: do not return the name length
HID: logitech-hidpp: check name retrieval return code
HID: logitech-hidpp: add boundary check for name retrieval
HID: logitech-hidpp: disable io in probe error path
drivers/hid/hid-logitech-hidpp.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
--
2.1.3
We do not make any use of the actual name length get through
hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
patch also replaces a (now) unnecessary goto by return NULL.
Signed-off-by: Peter Wu <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1a6395d..5066df8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
return count;
}
-static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
+static char *hidpp_get_device_name(struct hidpp_device *hidpp)
{
u8 feature_type;
u8 feature_index;
@@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
&feature_index, &feature_type);
if (ret)
- goto out_err;
+ return NULL;
ret = hidpp_devicenametype_get_count(hidpp, feature_index,
&__name_length);
if (ret)
- goto out_err;
+ return NULL;
name = kzalloc(__name_length + 1, GFP_KERNEL);
if (!name)
- goto out_err;
+ return NULL;
- *name_length = __name_length + 1;
while (index < __name_length)
index += hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
__name_length - index);
return name;
-
-out_err:
- *name_length = 0;
- return NULL;
}
/* -------------------------------------------------------------------------- */
@@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
char *name;
- u8 name_length;
if (use_unifying)
/*
@@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
*/
name = hidpp_get_unifying_name(hidpp);
else
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name)
hid_err(hdev, "unable to retrieve the name of the device");
@@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
bool connected = atomic_read(&hidpp->connected);
struct input_dev *input;
char *name, *devm_name;
- u8 name_length;
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
wtp_connect(hdev, connected);
@@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
return;
}
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name) {
hid_err(hdev, "unable to retrieve the name of the device");
} else {
--
2.1.3
Balance a hid_device_io_start() call with hid_device_io_stop() in the
error path. This avoids processing of HID reports when the probe fails
which possibly leads to invalid memory access in hid_device_probe() as
report_enum->report_id_hash might already be freed via
hid_close_report().
hid_set_drvdata() is called before wtp_allocate, be consistent and clear
drvdata too on the error path of wtp_allocate.
Signed-off-by: Peter Wu <[email protected]>
---
Hi Andrew,
There are few users of hid_device_io_start/stop, this is the first one
to get start/stop out of sync. Should the comment of
hid_device_io_start() be updated to ensure that hid_device_io_stop()
gets called before probe() returns? Or should the hid-core be changed to
handle this out-of-sync issue:
if (ret) {
if (hdev->io_started))
down(&hdev->driver_input_lock);
hid_close_report(hdev);
hdev->driver = NULL;
}
Is my observation correct or not that HID reports can arrive during
hid_close_report() when io_started == true?
Kind regards,
Peter
---
drivers/hid/hid-logitech-hidpp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4292cc3..2f420c0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_allocate(hdev, id);
if (ret)
- return ret;
+ goto wtp_allocate_fail;
}
INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
if (!connected) {
hid_err(hdev, "Device not connected");
+ hid_device_io_stop(hdev);
goto hid_parse_fail;
}
@@ -1186,6 +1187,7 @@ hid_hw_start_fail:
hid_parse_fail:
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
+wtp_allocate_fail:
hid_set_drvdata(hdev, NULL);
return ret;
}
--
2.1.3
The HID response has a limited size. Do not trust the value returned by
hardware, check that it really fits in the message.
Signed-off-by: Peter Wu <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4d72c20..4292cc3 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
len = response.rap.params[1];
+ if (2 + len > sizeof(response.rap.params))
+ return NULL;
+
name = kzalloc(len + 1, GFP_KERNEL);
if (!name)
return NULL;
--
2.1.3
hidpp_devicenametype_get_device_name() may return a negative value on
protocol errors (for example, when the device is powered off).
Explicitly check this condition to avoid a long-running loop.
(0 cannot be returned as __name_length - index > 0, but check for it
anyway as it would otherwise result in an infinite loop.)
Signed-off-by: Peter Wu <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5066df8..4d72c20 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
if (!name)
return NULL;
- while (index < __name_length)
- index += hidpp_devicenametype_get_device_name(hidpp,
+ while (index < __name_length) {
+ ret = hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
__name_length - index);
+ if (ret <= 0) {
+ kfree(name);
+ return NULL;
+ }
+ index += ret;
+ }
return name;
}
--
2.1.3
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> We do not make any use of the actual name length get through
> hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
> patch also replaces a (now) unnecessary goto by return NULL.
>
> Signed-off-by: Peter Wu <[email protected]>
> ---
Signed-off-by: Benjamin Tissoires <[email protected]>
Thanks for the v2 Peter.
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1a6395d..5066df8 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> return count;
> }
>
> -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> {
> u8 feature_type;
> u8 feature_index;
> @@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
> &feature_index, &feature_type);
> if (ret)
> - goto out_err;
> + return NULL;
>
> ret = hidpp_devicenametype_get_count(hidpp, feature_index,
> &__name_length);
> if (ret)
> - goto out_err;
> + return NULL;
>
> name = kzalloc(__name_length + 1, GFP_KERNEL);
> if (!name)
> - goto out_err;
> + return NULL;
>
> - *name_length = __name_length + 1;
> while (index < __name_length)
> index += hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
> __name_length - index);
>
> return name;
> -
> -out_err:
> - *name_length = 0;
> - return NULL;
> }
>
> /* -------------------------------------------------------------------------- */
> @@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> {
> struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> char *name;
> - u8 name_length;
>
> if (use_unifying)
> /*
> @@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> */
> name = hidpp_get_unifying_name(hidpp);
> else
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
>
> if (!name)
> hid_err(hdev, "unable to retrieve the name of the device");
> @@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> bool connected = atomic_read(&hidpp->connected);
> struct input_dev *input;
> char *name, *devm_name;
> - u8 name_length;
>
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> wtp_connect(hdev, connected);
> @@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> return;
> }
>
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
> if (!name) {
> hid_err(hdev, "unable to retrieve the name of the device");
> } else {
> --
> 2.1.3
>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> hidpp_devicenametype_get_device_name() may return a negative value on
> protocol errors (for example, when the device is powered off).
> Explicitly check this condition to avoid a long-running loop.
>
> (0 cannot be returned as __name_length - index > 0, but check for it
> anyway as it would otherwise result in an infinite loop.)
>
> Signed-off-by: Peter Wu <[email protected]>
> ---
Reviewed-by: Benjamin Tissoires <[email protected]>
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 5066df8..4d72c20 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> if (!name)
> return NULL;
>
> - while (index < __name_length)
> - index += hidpp_devicenametype_get_device_name(hidpp,
> + while (index < __name_length) {
> + ret = hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
> __name_length - index);
> + if (ret <= 0) {
> + kfree(name);
> + return NULL;
> + }
> + index += ret;
> + }
>
> return name;
> }
> --
> 2.1.3
>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> Balance a hid_device_io_start() call with hid_device_io_stop() in the
> error path. This avoids processing of HID reports when the probe fails
> which possibly leads to invalid memory access in hid_device_probe() as
> report_enum->report_id_hash might already be freed via
> hid_close_report().
Well spotted too!
>
> hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> drvdata too on the error path of wtp_allocate.
This is not strictly speaking required. We will have a dangling value in
hdev->private_data, but this should be overwritten before the next use.
Anyway, it makes sense to clean up after a failure, so the patch is
Reviewed-by: Benjamin Tissoires <[email protected]>
Cheers,
Benjamin
>
> Signed-off-by: Peter Wu <[email protected]>
> ---
> Hi Andrew,
>
> There are few users of hid_device_io_start/stop, this is the first one
> to get start/stop out of sync. Should the comment of
> hid_device_io_start() be updated to ensure that hid_device_io_stop()
> gets called before probe() returns? Or should the hid-core be changed to
> handle this out-of-sync issue:
>
> if (ret) {
> if (hdev->io_started))
> down(&hdev->driver_input_lock);
> hid_close_report(hdev);
> hdev->driver = NULL;
> }
>
> Is my observation correct or not that HID reports can arrive during
> hid_close_report() when io_started == true?
>
> Kind regards,
> Peter
> ---
> drivers/hid/hid-logitech-hidpp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4292cc3..2f420c0 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> ret = wtp_allocate(hdev, id);
> if (ret)
> - return ret;
> + goto wtp_allocate_fail;
> }
>
> INIT_WORK(&hidpp->work, delayed_work_cb);
> @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> if (!connected) {
> hid_err(hdev, "Device not connected");
> + hid_device_io_stop(hdev);
> goto hid_parse_fail;
> }
>
> @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> hid_parse_fail:
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> +wtp_allocate_fail:
> hid_set_drvdata(hdev, NULL);
> return ret;
> }
> --
> 2.1.3
>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> The HID response has a limited size. Do not trust the value returned by
> hardware, check that it really fits in the message.
>
> Signed-off-by: Peter Wu <[email protected]>
> ---
Well spotted!
Reviewed-by: Benjamin Tissoires <[email protected]>
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4d72c20..4292cc3 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
>
> len = response.rap.params[1];
>
> + if (2 + len > sizeof(response.rap.params))
> + return NULL;
> +
> name = kzalloc(len + 1, GFP_KERNEL);
> if (!name)
> return NULL;
> --
> 2.1.3
>
On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > error path. This avoids processing of HID reports when the probe fails
> > which possibly leads to invalid memory access in hid_device_probe() as
> > report_enum->report_id_hash might already be freed via
> > hid_close_report().
>
> Well spotted too!
>
> >
> > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > drvdata too on the error path of wtp_allocate.
>
> This is not strictly speaking required. We will have a dangling value in
> hdev->private_data, but this should be overwritten before the next use.
>
> Anyway, it makes sense to clean up after a failure, so the patch is
> Reviewed-by: Benjamin Tissoires <[email protected]>
>
> Cheers,
> Benjamin
>
> >
> > Signed-off-by: Peter Wu <[email protected]>
> > ---
> > Hi Andrew,
> >
> > There are few users of hid_device_io_start/stop, this is the first one
> > to get start/stop out of sync. Should the comment of
> > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > gets called before probe() returns? Or should the hid-core be changed to
> > handle this out-of-sync issue:
I do not have a strong opinion on this, and will defer to others. The
reason I needed to communicate during probe() was to have the driver
probe the actual device for details. In this use case, I would be okay
to disable IO at the end of probe() and have it become reenabled via
the normal (default) methods.
-andrew
> >
> > if (ret) {
> > if (hdev->io_started))
> > down(&hdev->driver_input_lock);
> > hid_close_report(hdev);
> > hdev->driver = NULL;
> > }
> >
> > Is my observation correct or not that HID reports can arrive during
> > hid_close_report() when io_started == true?
> >
> > Kind regards,
> > Peter
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 4292cc3..2f420c0 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > ret = wtp_allocate(hdev, id);
> > if (ret)
> > - return ret;
> > + goto wtp_allocate_fail;
> > }
> >
> > INIT_WORK(&hidpp->work, delayed_work_cb);
> > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> > if (!connected) {
> > hid_err(hdev, "Device not connected");
> > + hid_device_io_stop(hdev);
> > goto hid_parse_fail;
> > }
> >
> > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> > hid_parse_fail:
> > cancel_work_sync(&hidpp->work);
> > mutex_destroy(&hidpp->send_mutex);
> > +wtp_allocate_fail:
> > hid_set_drvdata(hdev, NULL);
> > return ret;
> > }
> > --
> > 2.1.3
> >
> --
> 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
On Thursday 11 December 2014 09:37:06 Andrew de los Reyes wrote:
> On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > > error path. This avoids processing of HID reports when the probe fails
> > > which possibly leads to invalid memory access in hid_device_probe() as
> > > report_enum->report_id_hash might already be freed via
> > > hid_close_report().
> >
> > Well spotted too!
> >
> > >
> > > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > > drvdata too on the error path of wtp_allocate.
> >
> > This is not strictly speaking required. We will have a dangling value in
> > hdev->private_data, but this should be overwritten before the next use.
> >
> > Anyway, it makes sense to clean up after a failure, so the patch is
> > Reviewed-by: Benjamin Tissoires <[email protected]>
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Signed-off-by: Peter Wu <[email protected]>
> > > ---
> > > Hi Andrew,
> > >
> > > There are few users of hid_device_io_start/stop, this is the first one
> > > to get start/stop out of sync. Should the comment of
> > > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > > gets called before probe() returns? Or should the hid-core be changed to
> > > handle this out-of-sync issue:
>
> I do not have a strong opinion on this, and will defer to others. The
> reason I needed to communicate during probe() was to have the driver
> probe the actual device for details. In this use case, I would be okay
> to disable IO at the end of probe() and have it become reenabled via
> the normal (default) methods.
>
> -andrew
Keeping the reports enabled when the probe succeeds is fine, I am
referring to the error path. If the probe fails, then reports should
never be accepted or a corruption may occur (if I see it correctly).
Is this analysis correct?
hid_device_probe()
hid_device_io_start()
return FAILURE
hid_close_report(device)
report_enum = device
->report_enum[i]
hid_free_report(report_enum
->report_id_hash[j]) <-- NOTE: freed
... interrupt occurs ...
hid_input_report()
hid_get_report()
report = report_enum->report_id_hash[n];
^ NOTE: use-after-free
return report if not NULL
hdrv->raw_event(report) <--- BOOM?
kfree(device->rdesc)
device->driver = NULL
Kind regards,
Peter
> > >
> > > if (ret) {
> > > if (hdev->io_started))
> > > down(&hdev->driver_input_lock);
> > > hid_close_report(hdev);
> > > hdev->driver = NULL;
> > > }
> > >
> > > Is my observation correct or not that HID reports can arrive during
> > > hid_close_report() when io_started == true?
> > >
> > > Kind regards,
> > > Peter
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 4292cc3..2f420c0 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > > ret = wtp_allocate(hdev, id);
> > > if (ret)
> > > - return ret;
> > > + goto wtp_allocate_fail;
> > > }
> > >
> > > INIT_WORK(&hidpp->work, delayed_work_cb);
> > > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> > > if (!connected) {
> > > hid_err(hdev, "Device not connected");
> > > + hid_device_io_stop(hdev);
> > > goto hid_parse_fail;
> > > }
> > >
> > > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> > > hid_parse_fail:
> > > cancel_work_sync(&hidpp->work);
> > > mutex_destroy(&hidpp->send_mutex);
> > > +wtp_allocate_fail:
> > > hid_set_drvdata(hdev, NULL);
> > > return ret;
> > > }
> > > --
> > > 2.1.3
> > >
On Thu, 11 Dec 2014, Peter Wu wrote:
> Hi Jiri,
>
> Here are four patches intended for the 3.19 stream and are based on
> jikos/hid (for-next, v3.18-rc4-144-gd9372ee).
>
> * The first is actually from Benjamin Tissoires, but modified to remove
> a now unneeded goto.
> * The second one depends on the first (it could work without, but there
> will be a context mismatch).
> * The third one can be applied independently of the others and is needed
> to avoid a possible buffer overread.
> * The fourth and final patch fixes an unbalanced hid_device_io_start().
>
> Tested by booting with three paired USB devices (QEMU + USB
> passthrough), two of them are powered off and one M525 is active. evbug
> registers mouse events.
>
Good stuff, thanks!
Applied.
--
Jiri Kosina
SUSE Labs
I would think you'd need to call hid_device_io_stop in probe before
returning just to make sure the hid->driver_input_lock is in the right
state. You may be able to consult hid->io_started in the probe thread
after calling probe, and do the appropriate cleanup if probe returned
error. IMHO I like to keep things like lock/unlock in pairs near each
other, so it seems preferable to require that drivers handle calling
hid_device_io_stop themselves if probe will fail. I don't know in
general what the Linux stance is, tho.
-andrew
On Thu, Dec 11, 2014 at 9:53 AM, Peter Wu <[email protected]> wrote:
> On Thursday 11 December 2014 09:37:06 Andrew de los Reyes wrote:
>> On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
>> <[email protected]> wrote:
>> >
>> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
>> > > Balance a hid_device_io_start() call with hid_device_io_stop() in the
>> > > error path. This avoids processing of HID reports when the probe fails
>> > > which possibly leads to invalid memory access in hid_device_probe() as
>> > > report_enum->report_id_hash might already be freed via
>> > > hid_close_report().
>> >
>> > Well spotted too!
>> >
>> > >
>> > > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
>> > > drvdata too on the error path of wtp_allocate.
>> >
>> > This is not strictly speaking required. We will have a dangling value in
>> > hdev->private_data, but this should be overwritten before the next use.
>> >
>> > Anyway, it makes sense to clean up after a failure, so the patch is
>> > Reviewed-by: Benjamin Tissoires <[email protected]>
>> >
>> > Cheers,
>> > Benjamin
>> >
>> > >
>> > > Signed-off-by: Peter Wu <[email protected]>
>> > > ---
>> > > Hi Andrew,
>> > >
>> > > There are few users of hid_device_io_start/stop, this is the first one
>> > > to get start/stop out of sync. Should the comment of
>> > > hid_device_io_start() be updated to ensure that hid_device_io_stop()
>> > > gets called before probe() returns? Or should the hid-core be changed to
>> > > handle this out-of-sync issue:
>>
>> I do not have a strong opinion on this, and will defer to others. The
>> reason I needed to communicate during probe() was to have the driver
>> probe the actual device for details. In this use case, I would be okay
>> to disable IO at the end of probe() and have it become reenabled via
>> the normal (default) methods.
>>
>> -andrew
>
> Keeping the reports enabled when the probe succeeds is fine, I am
> referring to the error path. If the probe fails, then reports should
> never be accepted or a corruption may occur (if I see it correctly).
>
> Is this analysis correct?
>
> hid_device_probe()
> hid_device_io_start()
> return FAILURE
> hid_close_report(device)
> report_enum = device
> ->report_enum[i]
> hid_free_report(report_enum
> ->report_id_hash[j]) <-- NOTE: freed
> ... interrupt occurs ...
> hid_input_report()
> hid_get_report()
> report = report_enum->report_id_hash[n];
> ^ NOTE: use-after-free
> return report if not NULL
> hdrv->raw_event(report) <--- BOOM?
> kfree(device->rdesc)
> device->driver = NULL
>
> Kind regards,
> Peter
>
>> > >
>> > > if (ret) {
>> > > if (hdev->io_started))
>> > > down(&hdev->driver_input_lock);
>> > > hid_close_report(hdev);
>> > > hdev->driver = NULL;
>> > > }
>> > >
>> > > Is my observation correct or not that HID reports can arrive during
>> > > hid_close_report() when io_started == true?
>> > >
>> > > Kind regards,
>> > > Peter
>> > > ---
>> > > drivers/hid/hid-logitech-hidpp.c | 4 +++-
>> > > 1 file changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > > index 4292cc3..2f420c0 100644
>> > > --- a/drivers/hid/hid-logitech-hidpp.c
>> > > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> > > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>> > > ret = wtp_allocate(hdev, id);
>> > > if (ret)
>> > > - return ret;
>> > > + goto wtp_allocate_fail;
>> > > }
>> > >
>> > > INIT_WORK(&hidpp->work, delayed_work_cb);
>> > > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> > > if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
>> > > if (!connected) {
>> > > hid_err(hdev, "Device not connected");
>> > > + hid_device_io_stop(hdev);
>> > > goto hid_parse_fail;
>> > > }
>> > >
>> > > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
>> > > hid_parse_fail:
>> > > cancel_work_sync(&hidpp->work);
>> > > mutex_destroy(&hidpp->send_mutex);
>> > > +wtp_allocate_fail:
>> > > hid_set_drvdata(hdev, NULL);
>> > > return ret;
>> > > }
>> > > --
>> > > 2.1.3
>> > >
>