linux-kernel - Re: [PATCH v2 8/9] platform/surface: Add Surface Aggregator user-space interface

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: <a22def75-a4e5-4ae4-f527-7695630be1ee@gmail.com>
Date:   Tue, 15 Dec 2020 21:00:16 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org
Cc:     Mark Gross <mgross@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Blaž Hrastnik <blaz@...n.io>,
        Dorian Stoll <dorian.stoll@...p.io>,
        platform-driver-x86@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 8/9] platform/surface: Add Surface Aggregator
 user-space interface

On 12/15/20 5:35 PM, Hans de Goede wrote:
> Hi,
> 
> On 12/3/20 10:26 PM, Maximilian Luz wrote:
>> Add a misc-device providing user-space access to the Surface Aggregator
>> EC, mainly intended for debugging, testing, and reverse-engineering.
>> This interface gives user-space applications the ability to send
>> requests to the EC and receive the corresponding responses.
>>
>> The device-file is managed by a pseudo platform-device and corresponding
>> driver to avoid dependence on the dedicated bus, allowing it to be
>> loaded in a minimal configuration.
>>
>> Signed-off-by: Maximilian Luz <luzmaximilian@...il.com>
> 
> 1 review comment inline:
>

[...]

>> +static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
>> +{
>> +	struct ssam_cdev_request __user *r;
>> +	struct ssam_cdev_request rqst;
>> +	struct ssam_request spec;
>> +	struct ssam_response rsp;
>> +	const void __user *plddata;
>> +	void __user *rspdata;
>> +	int status = 0, ret = 0, tmp;
>> +
>> +	r = (struct ssam_cdev_request __user *)arg;
>> +	ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
>> +	if (ret)
>> +		goto out;
>> +
>> +	plddata = u64_to_user_ptr(rqst.payload.data);
>> +	rspdata = u64_to_user_ptr(rqst.response.data);
>> +
>> +	/* Setup basic request fields. */
>> +	spec.target_category = rqst.target_category;
>> +	spec.target_id = rqst.target_id;
>> +	spec.command_id = rqst.command_id;
>> +	spec.instance_id = rqst.instance_id;
>> +	spec.flags = rqst.flags;
>> +	spec.length = rqst.payload.length;
>> +	spec.payload = NULL;
>> +
>> +	rsp.capacity = rqst.response.length;
>> +	rsp.length = 0;
>> +	rsp.pointer = NULL;
>> +
>> +	/* Get request payload from user-space. */
>> +	if (spec.length) {
>> +		if (!plddata) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		spec.payload = kzalloc(spec.length, GFP_KERNEL);
>> +		if (!spec.payload) {
>> +			status = -ENOMEM;
>> +			ret = -EFAULT;
>> +			goto out;
>> +		}
>> +
>> +		if (copy_from_user((void *)spec.payload, plddata, spec.length)) {
>> +			ret = -EFAULT;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	/* Allocate response buffer. */
>> +	if (rsp.capacity) {
>> +		if (!rspdata) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
>> +		if (!rsp.pointer) {
>> +			status = -ENOMEM;
>> +			ret = -EFAULT;
> 
> This is weird, -EFAULT should only be used if a SEGFAULT
> would have been raised if the code was running in
> userspace rather then in kernelspace, IOW if userspace
> has provided an invalid pointer (or a too small buffer,
> causing the pointer to become invalid at some point in
> the buffer).

Oh, right.

> IMHO you should simply do ret = -ENOMEM here.

Yes. that looks better. I will change that as suggested.
> Otherwise this looks good to me.

Thanks,
Max