On Tue, Jun 21, 2022 at 10:55 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> 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.
Thanks Dan. I will do it next time.
I am just playing fun with the built-in coccinelle script. Static
analysis has many false positives. Sorry for my false alarm.
Thanks for your valuable feedback.
>
> 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
|