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 >
