| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cf5213f9-0863-eba6-0e74-2f15577dba9d@gmail.com>
Date: Mon, 11 Jan 2021 14:55:43 +0100
From: Maximilian Luz <luzmaximilian@...il.com>
To: Colin Ian King <colin.king@...onical.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
Mark Gross <mgross@...ux.intel.com>,
platform-driver-x86@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: platform/surface: Add Surface Aggregator user-space interface
(static analysis issues)
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@...il.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
Powered by blists - more mailing lists