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.
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.
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
