[PATCH] Staging: bcm: Fix information leak in ioctl, IOCTL_BCM_REGISTER_READ_PRIVATE
Dan Carpenter
dan.carpenter at oracle.com
Wed Nov 2 06:52:34 UTC 2011
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
regards,
dan carpenter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20111102/519e52d7/attachment.asc>
More information about the devel
mailing list