| Message ID | 20210524095039.9033-1-lyl2019@mail.ustc.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: be2iscsi: Fix a use after free in beiscsi_if_clr_ip | expand |
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().
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.
diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c index 462717bbb5b7..f05fcea707cd 100644 --- a/drivers/scsi/be2iscsi/be_mgmt.c +++ b/drivers/scsi/be2iscsi/be_mgmt.c @@ -509,6 +509,7 @@ beiscsi_if_clr_ip(struct beiscsi_hba *phba, { struct be_cmd_set_ip_addr_req *req; struct be_dma_mem nonemb_cmd; + u32 status; int rc; rc = beiscsi_prep_nemb_cmd(phba, &nonemb_cmd, CMD_SUBSYSTEM_ISCSI, @@ -531,11 +532,12 @@ beiscsi_if_clr_ip(struct beiscsi_hba *phba, memcpy(req->ip_params.ip_record.ip_addr.subnet_mask, if_info->ip_addr.subnet_mask, sizeof(if_info->ip_addr.subnet_mask)); + status = req->ip_params.ip_record.status; rc = beiscsi_exec_nemb_cmd(phba, &nonemb_cmd, NULL, NULL, 0); - if (rc < 0 || req->ip_params.ip_record.status) { + if (rc < 0 || status) { beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG, "BG_%d : failed to clear IP: rc %d status %d\n", - rc, req->ip_params.ip_record.status); + rc, status); } return rc; }
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. Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> --- drivers/scsi/be2iscsi/be_mgmt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)