Bug 189141
| Summary: | Function bnx2x_init_firmware() does not set error code when the call to kmalloc() fails | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | bianpan (bianpan2010) |
| Component: | Network | Assignee: | drivers_network (drivers_network) |
| Status: | RESOLVED CODE_FIX | ||
| Severity: | normal | ||
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | linux-4.9-rc6 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
| Attachments: | The patch fixes the bug | ||
Created attachment 256463 [details]
The patch fixes the bug
The patch has been merged into the latest version of the Linux kernel. So I will close the bug.
|
Function bnx2x_init_firmware() is defined in file drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c. It calls macro BNX2X_ALLOC_AND_SET() (at lines 13508, 13511, 13514, and 13535) to allocate memory. In macro BNX2X_ALLOC_AND_SET(), kmalloc() is called, and if it returns a NULL pointer, the control flow will jump to the given label, such as "request_firmware_exit", and returns variable rc at last. Because variable rc is checked at line 13499, its value must be 0 when BNX2X_ALLOC_AND_SET() is called. As a result, the return value will be 0 (indicates success) even if memory allocation fails. The inconsistence between return value and execution status may misled the callers of bnx2x_init_firmware(), resulting in unexpected bugs. Though these errors may occur rarely, I think it's better to return correct error code on failures. Codes related to these bugs are summarised as follows. bnx2x_init_firmware @@ drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c 13460 #define BNX2X_ALLOC_AND_SET(arr, lbl, func) \ 13461 do { \ 13462 u32 len = be32_to_cpu(fw_hdr->arr.len); \ 13463 bp->arr = kmalloc(len, GFP_KERNEL); \ 13464 if (!bp->arr) \ 13465 goto lbl; \ 13466 func(bp->firmware->data + be32_to_cpu(fw_hdr->arr.offset), \ 13467 (u8 *)bp->arr, len); \ 13468 } while (0) 13469 13470 static int bnx2x_init_firmware(struct bnx2x *bp) 13471 { 13472 const char *fw_file_name; 13473 struct bnx2x_fw_file_hdr *fw_hdr; 13474 int rc; ... 13498 rc = bnx2x_check_firmware(bp); 13499 if (rc) { 13500 BNX2X_ERR("Corrupt firmware file %s\n", fw_file_name); 13501 goto request_firmware_exit; 13502 } 13503 13504 fw_hdr = (struct bnx2x_fw_file_hdr *)bp->firmware->data; 13505 13506 /* Initialize the pointers to the init arrays */ 13507 /* Blob */ // Insert "rc = -ENOMEM;" ? // Bug (1) 13508 BNX2X_ALLOC_AND_SET(init_data, request_firmware_exit, be32_to_cpu_n); 13509 13510 /* Opcodes */ // Bug (2) 13511 BNX2X_ALLOC_AND_SET(init_ops, init_ops_alloc_err, bnx2x_prep_ops); 13512 13513 /* Offsets */ // Bug (3) 13514 BNX2X_ALLOC_AND_SET(init_ops_offsets, init_offsets_alloc_err, 13515 be16_to_cpu_n); 13534 /* IRO */ // Bug (4) 13535 BNX2X_ALLOC_AND_SET(iro_arr, iro_alloc_err, bnx2x_prep_iro); 13536 13537 return 0; 13538 13539 iro_alloc_err: 13540 kfree(bp->init_ops_offsets); 13541 init_offsets_alloc_err: 13542 kfree(bp->init_ops); 13543 init_ops_alloc_err: 13544 kfree(bp->init_data); 13545 request_firmware_exit: 13546 release_firmware(bp->firmware); 13547 bp->firmware = NULL; 13548 13549 return rc; 13550 } Thanks very much!