On Tue, Jun 21, 2022 at 10:36:04PM +0800, Dongliang Mu wrote:
> Hi maintainers,
>
> I would like to send one bug report.
>
> In gb_bootrom_get_firmware, if the first branch is satisfied, it will
> go to queue_work, leading to the dereference of uninitialized const
> variable "fw". If the second branch is satisfied, it will go to unlock
> with fw as NULL pointer, leading to a NULL Pointer Dereference.
>
> The Fixes commit should be [1], introducing the dereference of "fw" in
> the error handling code.
>
> I am not sure how to fix this bug. Any comment on removing the
> dereference of fw?
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a4293e1d4e6416477976ee3bd248589d3fc4bb19
No, there is no bug there. It is a static checker false positive.
When you are reporting static checker warnings then please at least
mention it is from static analsysis so we can know what we are dealing
with.
Here is the code.
drivers/staging/greybus/bootrom.c
241 static int gb_bootrom_get_firmware(struct gb_operation *op)
242 {
243 struct gb_bootrom *bootrom = gb_connection_get_data(op->connection);
244 const struct firmware *fw;
^^^
245 struct gb_bootrom_get_firmware_request *firmware_request;
246 struct gb_bootrom_get_firmware_response *firmware_response;
247 struct device *dev = &op->connection->bundle->dev;
248 unsigned int offset, size;
249 enum next_request_type next_request;
250 int ret = 0;
251
252 /* Disable timeouts */
253 gb_bootrom_cancel_timeout(bootrom);
254
255 if (op->request->payload_size != sizeof(*firmware_request)) {
256 dev_err(dev, "%s: Illegal size of get firmware request (%zu %zu)\n",
257 __func__, op->request->payload_size,
258 sizeof(*firmware_request));
259 ret = -EINVAL;
260 goto queue_work;
"ret" is -EINVAL. "fw" is uninitialized.
261 }
262
263 mutex_lock(&bootrom->mutex);
264
265 fw = bootrom->fw;
266 if (!fw) {
267 dev_err(dev, "%s: firmware not available\n", __func__);
268 ret = -EINVAL;
269 goto unlock;
"ret" is -EINVAL. "fw" is NULL.
270 }
271
For the rest "fw" is valid.
272 firmware_request = op->request->payload;
273 offset = le32_to_cpu(firmware_request->offset);
274 size = le32_to_cpu(firmware_request->size);
275
276 if (offset >= fw->size || size > fw->size - offset) {
277 dev_warn(dev, "bad firmware request (offs = %u, size = %u)\n",
278 offset, size);
279 ret = -EINVAL;
280 goto unlock;
281 }
282
283 if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
284 GFP_KERNEL)) {
285 dev_err(dev, "%s: error allocating response\n", __func__);
286 ret = -ENOMEM;
287 goto unlock;
288 }
289
290 firmware_response = op->response->payload;
291 memcpy(firmware_response->data, fw->data + offset, size);
292
293 dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n",
294 offset, size);
295
296 unlock:
297 mutex_unlock(&bootrom->mutex);
298
299 queue_work:
300 /* Refresh timeout */
301 if (!ret && (offset + size == fw->size))
^^^^^
The "!ret" is only true when "fw" is valid.
302 next_request = NEXT_REQ_READY_TO_BOOT;
303 else
304 next_request = NEXT_REQ_GET_FIRMWARE;
305
306 gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
307
308 return ret;
309 }
regards,
dan carpenter
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|