* re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
@ 2021-01-11 12:12 Colin Ian King
2021-01-11 13:55 ` Maximilian Luz
0 siblings, 1 reply; 5+ messages in thread
From: Colin Ian King @ 2021-01-11 12:12 UTC (permalink / raw)
To: Maximilian Luz
Cc: Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel
Hi Maximilian,
Static analysis of linux-next with Coverity has found several issues
with the following commit:
commit 178f6ab77e617c984d6520b92e747075a12676ff
Author: Maximilian Luz <luzmaximilian@gmail.com>
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);
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);
157
158 return ret;
Colin
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
2021-01-11 12:12 platform/surface: Add Surface Aggregator user-space interface (static analysis issues) Colin Ian King
@ 2021-01-11 13:55 ` Maximilian Luz
2021-01-11 14:11 ` Colin Ian King
0 siblings, 1 reply; 5+ messages in thread
From: Maximilian Luz @ 2021-01-11 13:55 UTC (permalink / raw)
To: Colin Ian King
Cc: Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel
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@gmail.com>
> 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?
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
2021-01-11 13:55 ` Maximilian Luz
@ 2021-01-11 14:11 ` Colin Ian King
2021-01-11 14:37 ` Maximilian Luz
0 siblings, 1 reply; 5+ messages in thread
From: Colin Ian King @ 2021-01-11 14:11 UTC (permalink / raw)
To: Maximilian Luz
Cc: Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel
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@gmail.com>
>> 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.
>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
2021-01-11 14:11 ` Colin Ian King
@ 2021-01-11 14:37 ` Maximilian Luz
2021-01-11 14:45 ` Colin Ian King
0 siblings, 1 reply; 5+ messages in thread
From: Maximilian Luz @ 2021-01-11 14:37 UTC (permalink / raw)
To: Colin Ian King
Cc: Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel
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@gmail.com>
>>> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
2021-01-11 14:37 ` Maximilian Luz
@ 2021-01-11 14:45 ` Colin Ian King
0 siblings, 0 replies; 5+ messages in thread
From: Colin Ian King @ 2021-01-11 14:45 UTC (permalink / raw)
To: Maximilian Luz
Cc: Hans de Goede, Mark Gross, platform-driver-x86, linux-kernel
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@gmail.com>
>>>> 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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-11 14:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 12:12 platform/surface: Add Surface Aggregator user-space interface (static analysis issues) Colin Ian King
2021-01-11 13:55 ` Maximilian Luz
2021-01-11 14:11 ` Colin Ian King
2021-01-11 14:37 ` Maximilian Luz
2021-01-11 14:45 ` Colin Ian King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).