On Jul 16 2014 or thereabouts, Andrew Duggan wrote: > It is possible that the hid-rmi driver could get loaded onto a device which does not have the > expected report ids. This should not happen because it would indicate that the hid-rmi driver is > not compatible with that device. However, if it does happen it should return an error from probe > instead of dereferencing a null pointer. > > related bug: > https://bugzilla.kernel.org/show_bug.cgi?id=80091 > > Signed-off-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx> > --- Hi Andrew, I think this patch is required. We can not afford having a null pointer in a driver. So yes, I think Jiri should take this one. I just have a few nitpick inlined in the code. However, this patch will prevent the driver to fail, but the falsely bound device will not be driven by anybody, which means that you will break the keyboard/touchpad/whatever which is not using the right driver. So yes, we need that, but we also defintively have to fix hid-core too to not bind them to hid-rmi. > drivers/hid/hid-rmi.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c > index 3221a95..7f0927a 100644 > --- a/drivers/hid/hid-rmi.c > +++ b/drivers/hid/hid-rmi.c > @@ -848,6 +848,8 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id) > struct rmi_data *data = NULL; > int ret; > size_t alloc_size; > + struct hid_report *input_report; > + struct hid_report *output_report; > > data = devm_kzalloc(&hdev->dev, sizeof(struct rmi_data), GFP_KERNEL); > if (!data) > @@ -866,12 +868,24 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id) > return ret; > } > > - data->input_report_size = (hdev->report_enum[HID_INPUT_REPORT] > - .report_id_hash[RMI_ATTN_REPORT_ID]->size >> 3) > - + 1 /* report id */; > - data->output_report_size = (hdev->report_enum[HID_OUTPUT_REPORT] > - .report_id_hash[RMI_WRITE_REPORT_ID]->size >> 3) > - + 1 /* report id */; > + input_report = hdev->report_enum[HID_INPUT_REPORT] > + .report_id_hash[RMI_ATTN_REPORT_ID]; > + if (!input_report) { > + ret = -ENODEV; > + return ret; In its current state, the driver will be silently unbound, and the user will never get any explanations on why nothing works. So please add an hid_err message telling why hid-rmi gave up. > + } > + > + data->input_report_size = (input_report->size >> 3) + 1 /* report id */; > + > + output_report = hdev->report_enum[HID_OUTPUT_REPORT] > + .report_id_hash[RMI_WRITE_REPORT_ID]; > + if (!output_report) { > + ret = -ENODEV; > + return ret; ditto. Cheers, Benjamin > + } > + > + data->output_report_size = (output_report->size >> 3) > + + 1 /* report id */; > > alloc_size = data->output_report_size + data->input_report_size; > > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
