On 11/01/2021 14:37, Maximilian Luz wrote:
> On 1/11/21 3:11 PM, Colin Ian King wrote:
>> On 11/01/2021 13:55, Maximilian Luz wrote:
>>> On 1/11/21 1:12 PM, Colin Ian King wrote:
>>>> Hi Maximilian,
>>>>
>>>> Static analysis of linux-next with Coverity has found several issues
>>>> with the following commit:
>>>>
>>>> commit 178f6ab77e617c984d6520b92e747075a12676ff
>>>> Author: Maximilian Luz <luzmaximilian@xxxxxxxxx>
>>>> Date: Mon Dec 21 19:39:58 2020 +0100
>>>>
>>>> platform/surface: Add Surface Aggregator user-space interface
>>>>
>>>> The analysis is as follows:
>>>>
>>>> 65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long
>>>> arg)
>>>> 66{
>>>> 67 struct ssam_cdev_request __user *r;
>>>> 68 struct ssam_cdev_request rqst;
>>>>
>>>> 1. var_decl: Declaring variable spec without initializer.
>>>>
>>>> 69 struct ssam_request spec;
>>>> 70 struct ssam_response rsp;
>>>> 71 const void __user *plddata;
>>>> 72 void __user *rspdata;
>>>> 73 int status = 0, ret = 0, tmp;
>>>> 74
>>>> 75 r = (struct ssam_cdev_request __user *)arg;
>>>> 76 ret = copy_struct_from_user(&rqst, sizeof(rqst), r,
>>>> sizeof(*r));
>>>>
>>>> 2. Condition ret, taking true branch.
>>>>
>>>> 77 if (ret)
>>>>
>>>> 3. Jumping to label out.
>>>>
>>>> 78 goto out;
>>>>
>>>> 79
>>>> 80 plddata = u64_to_user_ptr(rqst.payload.data);
>>>> 81 rspdata = u64_to_user_ptr(rqst.response.data);
>>>> 82
>>>> 83 /* Setup basic request fields. */
>>>> 84 spec.target_category = rqst.target_category;
>>>> 85 spec.target_id = rqst.target_id;
>>>> 86 spec.command_id = rqst.command_id;
>>>> 87 spec.instance_id = rqst.instance_id;
>>>> 88 spec.flags = 0;
>>>> 89 spec.length = rqst.payload.length;
>>>> 90 spec.payload = NULL;
>>>> 91
>>>> 92 if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE)
>>>> 93 spec.flags |= SSAM_REQUEST_HAS_RESPONSE;
>>>> 94
>>>> 95 if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED)
>>>> 96 spec.flags |= SSAM_REQUEST_UNSEQUENCED;
>>>> 97
>>>> 98 rsp.capacity = rqst.response.length;
>>>> 99 rsp.length = 0;
>>>> 100 rsp.pointer = NULL;
>>>> 101
>>>> 102 /* Get request payload from user-space. */
>>>> 103 if (spec.length) {
>>>> 104 if (!plddata) {
>>>> 105 ret = -EINVAL;
>>>> 106 goto out;
>>>> 107 }
>>>> 108
>>>>
>>>> CID: Untrusted allocation size (TAINTED_SCALAR)
>>>> 8. tainted_data: Passing tainted expression spec.length to
>>>> kzalloc,
>>>> which uses it as an allocation size
>>>>
>>>> 109 spec.payload = kzalloc(spec.length, GFP_KERNEL);
>>>
>>> I assume a constraint on the maximum length will fix this?
>>
>> I believe so, it's unsigned so just an upper size check will be required
>> to silence this static analysis warning. Mind you, you may want a size
>> that is the full u16 max of 65535, so in that case the check is not
>> required.
>
> Right, the theoretical maximum payload (spec.length) and response size
> allowed by the Surface Aggregator SSH protocol is 'U16_MAX -
> sizeof(struct ssh_command)' (not that anything this size should ever be
> allocated in any normal case). Meaning it is (slightly) smaller than
> U16_MAX, but I'm not sure if it warrants a check here. The payload size
> is later validated by ssam_request_sync(), so it does only affect the
> allocation here (the response is just an output buffer and may be of
> arbitrary size).
>
> I think the limit imposed by having u16 as user-input should be enough.
Sounds good to me.
> I can still add an explicit check here if that is preferred, but I could
> also add a comment explaining that this should be safe.
If that's not too much trouble, that could be useful for folks later on
who look at this.
>
>>
>>>
>>>> 110 if (!spec.payload) {
>>>> 111 ret = -ENOMEM;
>>>> 112 goto out;
>>>> 113 }
>>>> 114
>>>> 115 if (copy_from_user((void *)spec.payload, plddata,
>>>> spec.length)) {
>>>> 116 ret = -EFAULT;
>>>> 117 goto out;
>>>> 118 }
>>>> 119 }
>>>> 120
>>>> 121 /* Allocate response buffer. */
>>>> 122 if (rsp.capacity) {
>>>> 123 if (!rspdata) {
>>>> 124 ret = -EINVAL;
>>>> 125 goto out;
>>>> 126 }
>>>> 127
>>>>
>>>> CID: Untrusted allocation size (TAINTED_SCALAR)
>>>> 12. tainted_data: Passing tainted expression rsp.capacity to
>>>> kzalloc,
>>>> which uses it as an allocation size
>>>>
>>>> 128 rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
>>>> 129 if (!rsp.pointer) {
>>>> 130 ret = -ENOMEM;
>>>> 131 goto out;
>>>> 132 }
>>>> 133 }
>>>> 134
>>>> 135 /* Perform request. */
>>>> 136 status = ssam_request_sync(cdev->ctrl, &spec, &rsp);
>>>> 137 if (status)
>>>> 138 goto out;
>>>> 139
>>>> 140 /* Copy response to user-space. */
>>>> 141 if (rsp.length && copy_to_user(rspdata, rsp.pointer,
>>>> rsp.length))
>>>> 142 ret = -EFAULT;
>>>> 143
>>>> 144out:
>>>> 145 /* Always try to set response-length and status. */
>>>>
>>>> CID: Uninitialized pointer read (UNINIT)
>>>> Using uninitialized value rsp.length
>>>>
>>>> 146 tmp = put_user(rsp.length, &r->response.length);
>>>>
>>>> 4. Condition tmp, taking true branch.
>>>>
>>>> 147 if (tmp)
>>>> 148 ret = tmp;
>>>> 149
>>>> 150 tmp = put_user(status, &r->status);
>>>>
>>>> 5. Condition tmp, taking true branch.
>>>>
>>>> 151 if (tmp)
>>>> 152 ret = tmp;
>>>> 153
>>>> 154 /* Cleanup. */
>>>>
>>>> CID: Uninitialized pointer read (UNINIT)
>>>> 6. uninit_use_in_call: Using uninitialized value spec.payload when
>>>> calling kfree.
>>>>
>>>> 155 kfree(spec.payload);
>>>>
>>>> CID: Uninitialized pointer read (UNINIT)
>>>> uninit_use_in_call: Using uninitialized value rsp.pointer when
>>>> calling kfree
>>>>
>>>> 156 kfree(rsp.pointer);
>>>
>>> Right, taking the first jump to out leaves rsp and spec uninitialized.
>>> I'll fix that.
>>>
>>>> 157
>>>> 158 return ret;
>>>>
>>>> Colin
>>>>
>>>
>>> Thank you for the analysis. I'll draft up two patches to address these
>>> issues.
>>>
>>> Regards,
>>> Max
>>
