Re: [uwb-i1480] question about value overwrite

Gustavo A. R. Silva
2017-05-19T08:14:06Z

Hi Greg,

Quoting Greg KH <gregkh@linuxfoundation.org>:

> 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 :)
>

Absolutely, I'll send it shortly.

Thanks!
--
Gustavo A. R. Silva