Re: [PATCH] Staging: bcm: Fix information leak in ioctl, IOCTL_BCM_REGISTER_READ_PRIVATE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,
On Wed, Nov 2, 2011 at 2:52 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> Ok.   I've found a different bug to complain about so this patch
> will need to be redone.  While you're at it, could you do some minor
> white space tweaks?
>
> On Mon, Oct 31, 2011 at 09:18:16PM -0400, Kevin McKinney wrote:
>> diff --git a/drivers/staging/bcm/InterfaceDld.c b/drivers/staging/bcm/InterfaceDld.c
>> index bcd86bb..19d84f6 100644
>> --- a/drivers/staging/bcm/InterfaceDld.c
>> +++ b/drivers/staging/bcm/InterfaceDld.c
>> @@ -62,6 +62,7 @@ int InterfaceFileReadbackFromChip(PVOID arg, struct file *flp, unsigned int on_c
>>       static int fw_down;
>>       INT Status = STATUS_SUCCESS;
>>       PS_INTERFACE_ADAPTER psIntfAdapter = (PS_INTERFACE_ADAPTER)arg;
>> +     INT retval = 0;
>                  ^^^^
>
> This initialization is not needed.  Use "int" instead of "INT".  Also
> I do hate the name "retval" and would really prefer you use bytes or
> count whenever you introduce it into a function that already has a
> Status.
>
>>
>>       buff = kmalloc(MAX_TRANSFER_CTRL_BYTE_USB, GFP_DMA);
>>       buff_readback = kmalloc(MAX_TRANSFER_CTRL_BYTE_USB , GFP_DMA);
>> @@ -94,10 +95,13 @@ int InterfaceFileReadbackFromChip(PVOID arg, struct file *flp, unsigned int on_c
>>                       break;
>>               }
>>
>> -             Status = InterfaceRDM(psIntfAdapter, on_chip_loc, buff_readback, len);
>> -             if (Status) {
>> +             retval = InterfaceRDM(psIntfAdapter, on_chip_loc, buff_readback, len);
>> +             if (retval < 0) {
>> +                     Status = retval;
>>                       BCM_DEBUG_PRINT(psIntfAdapter->psAdapter, DBG_TYPE_INITEXIT, MP_INIT, DBG_LVL_ALL, "RDM of len %d Failed! %d", len, reg);
>>                       goto exit;
>> +             } else {
>> +                     Status = STATUS_SUCCESS;
>
> Status is already STATUS_SUCCESS here so this is not needed.
>
>>               }
>>               reg++;
>>               if ((len-sizeof(unsigned int)) < 4) {
>> @@ -312,9 +316,11 @@ static INT buffRdbkVerify(PMINI_ADAPTER Adapter, PUCHAR mappedbuffer, UINT u32Fi
>>               len = MIN_VAL(u32FirmwareLength, MAX_TRANSFER_CTRL_BYTE_USB);
>>               retval = rdm(Adapter, u32StartingAddress, readbackbuff, len);
>>
>> -             if (retval) {
>> +             if (retval < 0) {
>>                       BCM_DEBUG_PRINT(Adapter, DBG_TYPE_INITEXIT, MP_INIT, DBG_LVL_ALL, "rdm failed with status %d", retval);
>>                       break;
>> +             } else {
>> +                     retval = STATUS_SUCCESS;
>>               }
>>
>>               retval = bcm_compare_buff_contents(readbackbuff, mappedbuffer, len);
>> diff --git a/drivers/staging/bcm/InterfaceIdleMode.c b/drivers/staging/bcm/InterfaceIdleMode.c
>> index 96fa4ea..49e0a91 100644
>> --- a/drivers/staging/bcm/InterfaceIdleMode.c
>> +++ b/drivers/staging/bcm/InterfaceIdleMode.c
>> @@ -46,6 +46,7 @@ int InterfaceIdleModeRespond(PMINI_ADAPTER Adapter, unsigned int* puiBuffer)
>>  {
>>       int     status = STATUS_SUCCESS;
>>       unsigned int    uiRegRead = 0;
>> +     int retval = 0;
>
> The initialization is not needed here.
>
>>
>>       BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, IDLE_MODE, DBG_LVL_ALL,"SubType of Message :0x%X", ntohl(*puiBuffer));
>>
>> @@ -77,18 +78,22 @@ int InterfaceIdleModeRespond(PMINI_ADAPTER Adapter, unsigned int* puiBuffer)
>>                       else if(Adapter->ulPowerSaveMode != DEVICE_POWERSAVE_MODE_AS_PROTOCOL_IDLE_MODE)
>>                       {
>>                               //clear on read Register
>> -                             status = rdmalt(Adapter, DEVICE_INT_OUT_EP_REG0, &uiRegRead, sizeof(uiRegRead));
>> -                             if(status)
>> -                             {
>> -                                     BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0, "rdm failed while clearing H/W Abort Reg0");
>> +                             retval = rdmalt(Adapter, DEVICE_INT_OUT_EP_REG0, &uiRegRead, sizeof(uiRegRead));
>> +                             if (retval < 0) {
>> +                                     status = retval;
>> +                                     BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "rdm failed while clearing H/W Abort Reg0");
>>                                       return status;
>> +                             } else {
>> +                                     status = STATUS_SUCCESS;
>
> Status is already STATUS_SUCCESS here.  STATUS_SUCCESS is zero btw,
> the code is checking it inconsistently in this function.  It normally
> checks status != STATUS_SUCCESS but here is checks if (status).  It's
> sort of messy but not related to your patch.
>
>>                               }
>>                               //clear on read Register
>> -                             status = rdmalt (Adapter, DEVICE_INT_OUT_EP_REG1, &uiRegRead, sizeof(uiRegRead));
>> -                             if(status)
>> -                             {
>> +                             retval = rdmalt(Adapter, DEVICE_INT_OUT_EP_REG1, &uiRegRead, sizeof(uiRegRead));
>> +                             if(retval < 0) {
>> +                                     status = retval;
>>                                       BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0, "rdm failed while clearing H/W Abort     Reg1");
>>                                       return status;
>> +                             } else {
>> +                                     status = STATUS_SUCCESS;
>
> Status is already STATUS_SUCCESS.
>
>>                               }
>>                       }
>>                       BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, IDLE_MODE, DBG_LVL_ALL, "Device Up from Idle Mode");
>> @@ -117,11 +122,13 @@ int InterfaceIdleModeRespond(PMINI_ADAPTER Adapter, unsigned int* puiBuffer)
>>                                       Adapter->chip_id== BCS220_3)
>>                       {
>>
>> -                             status = rdmalt(Adapter, HPM_CONFIG_MSW, &uiRegRead, sizeof(uiRegRead));
>> -                             if(status)
>> -                             {
>> -                                     BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, IDLE_MODE, DBG_LVL_ALL, "rdm failed while Reading HPM_CONFIG_LDO145 Reg 0\n");
>> +                             retval = rdmalt(Adapter, HPM_CONFIG_MSW, &uiRegRead, sizeof(uiRegRead));
>> +                             if(retval < 0) {
>> +                                     status = retval;
>> +                                     BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, IDLE_MODE, DBG_LVL_ALL, "rdm failed while Reading HPM_CONFIG_LDO145 Reg 0\n");
>>                                       return status;
>> +                             } else {
>> +                                     status = STATUS_SUCCESS;
>
> Status is already STATUS_SUCCESS.
>
>>                               }
>>
>>
>> @@ -130,7 +137,7 @@ int InterfaceIdleModeRespond(PMINI_ADAPTER Adapter, unsigned int* puiBuffer)
>>                               status = wrmalt (Adapter,HPM_CONFIG_MSW, &uiRegRead, sizeof(uiRegRead));
>>                               if(status)
>>                               {
>> -                                     BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0, "wrm failed while clearing Idle Mode Reg\n");
>> +                                     BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "wrm failed while clearing Idle Mode Reg\n");
>>                                       return status;
>>                               }
>>
>> @@ -266,6 +273,8 @@ void InterfaceHandleShutdownModeWakeup(PMINI_ADAPTER Adapter)
>>  {
>>       unsigned int uiRegVal = 0;
>>       INT Status = 0;
>> +     int retval = 0;
>> +
>>       if(Adapter->ulPowerSaveMode == DEVICE_POWERSAVE_MODE_AS_MANUAL_CLOCK_GATING)
>>       {
>>               // clear idlemode interrupt.
>> @@ -282,18 +291,22 @@ void InterfaceHandleShutdownModeWakeup(PMINI_ADAPTER Adapter)
>>       {
>>
>>          //clear Interrupt EP registers.
>> -             Status = rdmalt(Adapter,DEVICE_INT_OUT_EP_REG0, &uiRegVal, sizeof(uiRegVal));
>> -             if(Status)
>> -             {
>> -                     BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0,"RDM of DEVICE_INT_OUT_EP_REG0 failed with Err :%d", Status);
>> +             retval = rdmalt(Adapter, DEVICE_INT_OUT_EP_REG0, &uiRegVal, sizeof(uiRegVal));
>> +             if (retval < 0) {
>> +                     Status = retval;
>> +                     BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "RDM of DEVICE_INT_OUT_EP_REG0 failed with Err :%d", Status);
>>                       return;
>> +             } else {
>> +                     Status = STATUS_SUCCESS;
>
> Status is already STATUS_SUCCESS;
>
>>               }
>>
>> -        Status = rdmalt(Adapter,DEVICE_INT_OUT_EP_REG1, &uiRegVal, sizeof(uiRegVal));
>> -             if(Status)
>> -             {
>> -                     BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0,"RDM of DEVICE_INT_OUT_EP_REG1 failed with Err :%d", Status);
>> +             retval = rdmalt(Adapter, DEVICE_INT_OUT_EP_REG1, &uiRegVal, sizeof(uiRegVal));
>> +             if (retval < 0) {
>> +                     Status = retval;
>> +                     BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "RDM of DEVICE_INT_OUT_EP_REG1 failed with Err :%d", Status);
>>                       return;
>> +             } else {
>> +                     Status = STATUS_SUCCESS;
>
> Same.
>
>>               }
>>       }
>>  }
>> diff --git a/drivers/staging/bcm/InterfaceInit.c b/drivers/staging/bcm/InterfaceInit.c
>> index a09d351..4253143 100644
>> --- a/drivers/staging/bcm/InterfaceInit.c
>> +++ b/drivers/staging/bcm/InterfaceInit.c
>> @@ -95,10 +95,12 @@ static void ConfigureEndPointTypesThroughEEPROM(PMINI_ADAPTER Adapter)
>>
>>       /* Program TX EP as interrupt(Alternate Setting) */
>>       ret = rdmalt(Adapter, 0x0F0110F8, (u32 *)&ulReg, sizeof(u32));
>> -     if (ret) {
>> +     if (ret < 0) {
>>               BCM_DEBUG_PRINT(Adapter, DBG_TYPE_INITEXIT, DRV_ENTRY, DBG_LVL_ALL,
>>                       "reading of Tx EP failed\n");
>>               return;
>> +     } else {
>> +             ret = STATUS_SUCCESS;
>>       }
>>       ulReg |= 0x6;
>>
>> @@ -440,9 +442,11 @@ static int InterfaceAdapterInit(PS_INTERFACE_ADAPTER psIntfAdapter)
>>
>>       retval = rdmalt(psIntfAdapter->psAdapter, CHIP_ID_REG,
>>                       (u32 *)&(psIntfAdapter->psAdapter->chip_id), sizeof(u32));
>> -     if (retval) {
>> +     if (retval < 0) {
>>               BCM_DEBUG_PRINT(psIntfAdapter->psAdapter, DBG_TYPE_PRINTK, 0, 0, "CHIP ID Read Failed\n");
>>               return retval;
>> +     } else {
>> +             retval = STATUS_SUCCESS;
>>       }
>>
>>       if (0xbece3200 == (psIntfAdapter->psAdapter->chip_id & ~(0xF0)))
>> diff --git a/drivers/staging/bcm/InterfaceMisc.c b/drivers/staging/bcm/InterfaceMisc.c
>> index 61f878b..f10ecd8 100644
>> --- a/drivers/staging/bcm/InterfaceMisc.c
>> +++ b/drivers/staging/bcm/InterfaceMisc.c
>> @@ -48,15 +48,15 @@ INT InterfaceRDM(PS_INTERFACE_ADAPTER psIntfAdapter,
>>
>>       } while ((retval < 0) && (usRetries < MAX_RDM_WRM_RETIRES));
>>
>> -     if (retval < 0) {
>> +     if (retval < 0) {
>>               BCM_DEBUG_PRINT(psIntfAdapter->psAdapter, DBG_TYPE_OTHERS, RDM, DBG_LVL_ALL, "RDM failed status :%d, retires :%d", retval, usRetries);
>>               psIntfAdapter->psAdapter->DeviceAccess = FALSE;
>>               return retval;
>> -     } else {
>> -             BCM_DEBUG_PRINT(psIntfAdapter->psAdapter, DBG_TYPE_OTHERS, RDM, DBG_LVL_ALL, "RDM sent %d", retval);
>> -             psIntfAdapter->psAdapter->DeviceAccess = FALSE;
>> -             return STATUS_SUCCESS;
>>       }
>> +
>> +     BCM_DEBUG_PRINT(psIntfAdapter->psAdapter, DBG_TYPE_OTHERS, RDM, DBG_LVL_ALL, "RDM sent %d", retval);
>> +     psIntfAdapter->psAdapter->DeviceAccess = FALSE;
>> +     return retval;
>>  }
>>
>>  INT InterfaceWRM(PS_INTERFACE_ADAPTER psIntfAdapter,
>> diff --git a/drivers/staging/bcm/Misc.c b/drivers/staging/bcm/Misc.c
>> index e9f29d5..940aff9 100644
>> --- a/drivers/staging/bcm/Misc.c
>> +++ b/drivers/staging/bcm/Misc.c
>> @@ -852,6 +852,8 @@ int reset_card_proc(PMINI_ADAPTER ps_adapter)
>>                       if (retval < 0) {
>>                               BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "read failed with status :%d", retval);
>>                               goto err_exit;
>> +                     } else {
>> +                             retval = STATUS_SUCCESS;
>>                       }
>>                       /* setting 0th bit */
>>                       value |= (1<<0);
>> @@ -866,6 +868,8 @@ int reset_card_proc(PMINI_ADAPTER ps_adapter)
>>               if (retval < 0) {
>>                       BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "read failed with status :%d", retval);
>>                       goto err_exit;
>> +             } else {
>> +                     retval = STATUS_SUCCESS;
>>               }
>>               value &= (~(1<<16));
>>               retval = wrmalt(ps_adapter, 0x0f007018, &value, sizeof(value));
>> @@ -1232,11 +1236,13 @@ static unsigned char *ReadMacAddrEEPROM(PMINI_ADAPTER Adapter, ulong dwAddress)
>>
>>       for (i = 0; i < MAC_ADDRESS_SIZE; i++) {
>>               status = rdmalt(Adapter, EEPROM_READ_DATA_Q_REG, &temp, sizeof(temp));
>> -             if (status != STATUS_SUCCESS) {
>> +             if (status < 0) {
>>                       BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "rdm Failed..\n");
>>                       kfree(pucmacaddr);
>>                       pucmacaddr = NULL;
>>                       goto OUT;
>> +             } else {
>> +                     status = STATUS_SUCCESS;
>>               }
>>               pucmacaddr[i] = temp & 0xff;
>>               BCM_DEBUG_PRINT(Adapter, DBG_TYPE_INITEXIT, DRV_ENTRY, DBG_LVL_ALL, "%x\n", pucmacaddr[i]);
>> @@ -1346,6 +1352,8 @@ int rdmaltWithLock(PMINI_ADAPTER Adapter, UINT uiAddress, PUINT pucBuff, size_t
>>       }
>>
>>       uiRetVal = rdmalt(Adapter, uiAddress, pucBuff, size);
>> +     if (uiRetVal > 0)
>> +             uiRetVal = STATUS_SUCCESS;
>
>
> rdmaltWithLock() should return the same value that rdmalt() returns.
>
> This is the bug I mentioned at the start of the email.  (troll face).
>
>>  exit:
>>       up(&Adapter->rdmwrmsync);
>>       return uiRetVal;
>> diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
>> index 3de0daf..bb8fcf3 100644
>> --- a/drivers/staging/bcm/nvm.c
>> +++ b/drivers/staging/bcm/nvm.c
>> @@ -443,7 +443,7 @@ static INT BeceemFlashBulkRead(
>>  {
>>       UINT uiIndex = 0;
>>       UINT uiBytesToRead = uiNumBytes;
>> -     INT Status = 0;
>> +     int Status;
>>       UINT uiPartOffset = 0;
>>
>>       if(Adapter->device_removed )
>> @@ -469,11 +469,12 @@ static INT BeceemFlashBulkRead(
>>               uiBytesToRead = MAX_RW_SIZE - (uiOffset%MAX_RW_SIZE);
>>               uiBytesToRead = MIN(uiNumBytes,uiBytesToRead);
>>
>> -             if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
>> -             {
>> -                     Status = -1;
>> +             Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead);
>> +             if (Status < 0) {
>>                       Adapter->SelectedChip = RESET_CHIP_SELECT;
>>                       return Status;
>> +             } else {
>> +                     Status = STATUS_SUCCESS;
>>               }
>>
>>               uiIndex += uiBytesToRead;
>> @@ -488,12 +489,11 @@ static INT BeceemFlashBulkRead(
>>
>>               uiBytesToRead = MIN(uiNumBytes,MAX_RW_SIZE);
>>
>> -             if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
>> -             {
>> -                     Status = -1;
>> +             Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead);
>> +             if (Status < 0)
>>                       break;
>> -             }
>> -
>> +             else
>> +                     Status = STATUS_SUCCESS;
>>
>>               uiIndex += uiBytesToRead;
>>               uiOffset += uiBytesToRead;
>> @@ -613,6 +613,7 @@ static INT FlashSectorErase(PMINI_ADAPTER Adapter,
>>       UINT iIndex = 0, iRetries = 0;
>>       UINT uiStatus = 0;
>>       UINT value;
>> +     int status;
>>
>>       for(iIndex=0;iIndex<numOfSectors;iIndex++)
>>       {
>> @@ -632,10 +633,12 @@ static INT FlashSectorErase(PMINI_ADAPTER Adapter,
>>                               return STATUS_FAILURE;
>>                       }
>>
>> -                     if(rdmalt(Adapter, FLASH_SPI_READQ_REG, &uiStatus, sizeof(uiStatus)) < 0 )
>> -                     {
>> -                             BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0,"Reading status of FLASH_SPI_READQ_REG fails");
>> +                     status = rdmalt(Adapter, FLASH_SPI_READQ_REG, &uiStatus, sizeof(uiStatus));
>> +                     if (status < 0) {
>> +                             BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "Reading status of FLASH_SPI_READQ_REG fails");
>>                               return STATUS_FAILURE;
>
> It would be better to return status here instead of STATUS_FAILURE.
>
>> +                     } else {
>> +                             status = STATUS_SUCCESS;
>
> If you named the variable "bytes" here, and you left this bit out
> then it would look ok to leave this bit out.  You already have a
> uiStatus in this function so I hate the confusing duplication.
>
>>                       }
>>                       iRetries++;
>>                       //After every try lets make the CPU free for 10 ms. generally time taken by the
>
I will make these changes and resubmit this patch.

Thanks,
Kevin
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux