[PATCH] Staging: bcm: Fix information leak in ioctl, IOCTL_BCM_REGISTER_READ_PRIVATE
Kevin McKinney
klmckinney1 at gmail.com
Wed Nov 2 12:24:39 UTC 2011
Hi Dan,
On Wed, Nov 2, 2011 at 2:52 AM, Dan Carpenter <dan.carpenter at oracle.com> 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
More information about the devel
mailing list