On Thu, May 18, 2017 at 06:00:06PM -0500, Gustavo A. R. Silva wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1226913 I ran into the following piece of
> code at drivers/uwb/i1480/dfu/phy.c:99:
>
> 99static
> 100int i1480_mpi_read(struct i1480 *i1480, u8 *data, u16 srcaddr, size_t size)
> 101{
> 102 int result;
> 103 struct i1480_cmd_mpi_read *cmd = i1480->cmd_buf;
> 104 struct i1480_evt_mpi_read *reply = i1480->evt_buf;
> 105 unsigned cnt;
> 106
> 107 memset(i1480->cmd_buf, 0x69, 512);
> 108 memset(i1480->evt_buf, 0x69, 512);
> 109
> 110 BUG_ON(size > (i1480->buf_size - sizeof(*reply)) / 3);
> 111 result = -ENOMEM;
> 112 cmd->rccb.bCommandType = i1480_CET_VS1;
> 113 cmd->rccb.wCommand = cpu_to_le16(i1480_CMD_MPI_READ);
> 114 cmd->size = cpu_to_le16(3*size);
> 115 for (cnt = 0; cnt < size; cnt++) {
> 116 cmd->data[cnt].page = (srcaddr + cnt) >> 8;
> 117 cmd->data[cnt].offset = (srcaddr + cnt) & 0xff;
> 118 }
> 119 reply->rceb.bEventType = i1480_CET_VS1;
> 120 reply->rceb.wEvent = i1480_CMD_MPI_READ;
> 121 result = i1480_cmd(i1480, "MPI-READ", sizeof(*cmd) + 2*size,
> 122 sizeof(*reply) + 3*size);
> 123 if (result < 0)
> 124 goto out;
> 125 if (reply->bResultCode != UWB_RC_RES_SUCCESS) {
> 126 dev_err(i1480->dev, "MPI-READ: command execution failed:
> %d\n",
> 127 reply->bResultCode);
> 128 result = -EIO;
> 129 }
> 130 for (cnt = 0; cnt < size; cnt++) {
> 131 if (reply->data[cnt].page != (srcaddr + cnt) >> 8)
> 132 dev_err(i1480->dev, "MPI-READ: page inconsistency
> at "
> 133 "index %u: expected 0x%02x, got
> 0x%02x\n", cnt,
> 134 (srcaddr + cnt) >> 8,
> reply->data[cnt].page);
> 135 if (reply->data[cnt].offset != ((srcaddr + cnt) & 0x00ff))
> 136 dev_err(i1480->dev, "MPI-READ: offset
> inconsistency at "
> 137 "index %u: expected 0x%02x, got
> 0x%02x\n", cnt,
> 138 (srcaddr + cnt) & 0x00ff,
> 139 reply->data[cnt].offset);
> 140 data[cnt] = reply->data[cnt].value;
> 141 }
> 142 result = 0;
> 143out:
> 144 return result;
> 145}
>
> The issue is that the value store in variable _result_ at line 128 is
> overwritten by the one stored at line 142, before it can be used.
>
> My question is if the original intention was to return this value
> inmediately after the assignment at line 128, something like in the
> following patch:
>
> index 3b1a87d..1ac8526 100644
> --- a/drivers/uwb/i1480/dfu/phy.c
> +++ b/drivers/uwb/i1480/dfu/phy.c
> @@ -126,6 +126,7 @@ int i1480_mpi_read(struct i1480 *i1480, u8 *data, u16
> srcaddr, size_t size)
> dev_err(i1480->dev, "MPI-READ: command execution failed:
> %d\n",
> reply->bResultCode);
> result = -EIO;
> + goto out;
> }
> for (cnt = 0; cnt < size; cnt++) {
> if (reply->data[cnt].page != (srcaddr + cnt) >> 8)
>
> What do you think?
>
> I'd really appreciate any comment on this.
I think you are correct, I'll take a patch to fix this up if you want to
write one :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
