Messages in this thread | | | Subject | Re: [PATCH] scsi: be2iscsi: Fix a use after free in beiscsi_if_clr_ip | From | michael.christie@oracle ... | Date | Wed, 30 Jun 2021 20:13:59 -0500 |
| |
On 6/29/21 5:02 PM, Martin K. Petersen wrote: > > Lv, > >> In the free_cmd error path of callee beiscsi_exec_nemb_cmd(), >> nonemb_cmd->va is freed by dma_free_coherent(). As req = >> nonemb_cmd.va, we can see that the freed nonemb_cmd.va is still >> dereferenced and used by req->ip_params.ip_record.status. > >> My patch uses status to replace req->ip_params.ip_record.status to >> avoid the uaf. > > This status is captured prior to executing the command so it doesn't > actually reflect whether the operation was successful (which I believe > was the intent). > > Some of the callers of beiscsi_exec_nemb_cmd() pass a response buffer > and a response length as the two last arguments. Since > beiscsi_exec_nemb_cmd() frees the command before returning, passing a > response buffer seems to be the only way to get meaningful data out. > > I am not sure whether the OPCODE_COMMON_ISCSI_NTWK_MODIFY_IP_ADDR > operation returns something useful from the controller. As far as I can > tell not all operations have a response buffer defined. > > My recommendation would be to add a response buffer and try to decipher > what comes back from the firmware. Also, beiscsi_if_set_ip() appears to > have the same problem as beiscsi_if_clr_ip(). >
Hopefully the broadcom devs will chime in, but it doesn't look like we get a response buffer. I think we just need to move the:
dma_free_coherent(&ctrl->pdev->dev, nonemb_cmd->size, nonemb_cmd->va, nonemb_cmd->dma);
to a helper and have the callers free the cmd.
Oh yeah, I guess we haven't been able to hit this bug, because beiscsi_if_set_ip/beiscsi_if_clr_ip hasn't been run since 2013. This patch https://patchwork.kernel.org/project/linux-scsi/patch/20210701002559.89533-1-michael.christie@oracle.com/ allows us to be able to set the IP and hit those code paths.
| |