linux-kernel - Re: [PATCH] HID: rmi: check that report ids exist in the report_id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140717160704.GA27586@mail.corp.redhat.com>
Date:	Thu, 17 Jul 2014 12:07:05 -0400
From:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:	Andrew Duggan <aduggan@...aptics.com>
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH] HID: rmi: check that report ids exist in the
 report_id_hash before accessing their size

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@...aptics.com>
> ---

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/