* 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@vger.kernel.org
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@vger.kernel.org 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@vger.kernel.org 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@vger.kernel.org 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@vger.kernel.org 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