Bug 188941

Summary: Function beiscsi_create_cqs() may return improper value when the call to pci_alloc_consistent() fails, which may result in use-after-free
Product: SCSI Drivers Reporter: bianpan (bianpan2010)
Component: OtherAssignee: scsi_drivers-other
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

Description bianpan 2016-11-25 11:10:21 UTC
Function pci_alloc_consistent() returns a NULL pointer if there is no enough memory. In function beiscsi_create_cqs() defined in file drivers/scsi/be2iscsi/be_main.c, function pci_alloc_consistent() is called and its return value is checked against NULL (at line 3116). If the return value is NULL, the control flow will jump to label "create_cq_error", frees allocated memory and returns variable ret. Because after the first execution of the loop the value of ret must be 0 (see the check statement of ret at line 3129), the return value will be 0 (indicates success) if pci_alloc_consistent() fails during the second or after repeats of the loop body. In this case, the freed memory may be used or freed again in the callers of beiscsi_create_cqs(). I think it is better to assign "-ENOMEM" when the call pci_alloc_consistent() fails. Codes and comments related to this bug are summarised as follows.

beiscsi_create_cqs @@ drivers/scsi/be2iscsi/be_main.c
3092 static int beiscsi_create_cqs(struct beiscsi_hba *phba,
3093                  struct hwi_context_memory *phwi_context)
3094 {
         ...
3100     int ret = -ENOMEM;
         ...
3106     for (i = 0; i < phba->num_cpus; i++) {
3107         cq = &phwi_context->be_cq[i];
3108         eq = &phwi_context->be_eq[i].q;
3109         pbe_eq = &phwi_context->be_eq[i];
3110         pbe_eq->cq = cq;
3111         pbe_eq->phba = phba;
3112         mem = &cq->dma_mem;
3113         cq_vaddress = pci_alloc_consistent(phba->pcidev,
3114                            num_cq_pages * PAGE_SIZE,
3115                            &paddr);
3116         if (!cq_vaddress)
                 // ret may takes value 0. Add "ret = -ENOMEM" here?
3117             goto create_cq_error;
             ...
3129         ret = beiscsi_cmd_cq_create(&phba->ctrl, cq, eq, false,
3130                         false, 0);
3131         if (ret) {
3132             beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
3133                     "BM_%d : beiscsi_cmd_eq_create"
3134                     "Failed for ISCSI CQ\n");
3135             goto create_cq_error;
3136         }
3137         beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_INIT,
3138                 "BM_%d : iscsi cq_id is %d for eq_id %d\n"
3139                 "iSCSI CQ CREATED\n", cq->id, eq->id);
3140     }
3141     return 0;
3142 
3143 create_cq_error:
3144     for (i = 0; i < phba->num_cpus; i++) {
3145         cq = &phwi_context->be_cq[i];
3146         mem = &cq->dma_mem;
3147         if (mem->va)
3148             pci_free_consistent(phba->pcidev, num_cq_pages
3149                         * PAGE_SIZE,
3150                         mem->va, mem->dma);
3151     }
3152     return ret;
3153 }

Thanks very much!
Comment 1 bianpan 2017-05-12 00:14:25 UTC
Created attachment 256439 [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.