Messages in this thread | | | Subject | Re: [PATCH v2 8/9] platform/surface: Add Surface Aggregator user-space interface | From | Maximilian Luz <> | Date | Tue, 15 Dec 2020 21:00:16 +0100 |
| |
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@gmail.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
| |