Re: [uwb-i1480] question about value overwrite

[Gustavo A. R. Silva]

Hi Greg,

Quoting Greg KH :


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{
102int result;
103struct i1480_cmd_mpi_read *cmd = i1480->cmd_buf;
104struct i1480_evt_mpi_read *reply = i1480->evt_buf;
105unsigned cnt;
106
107memset(i1480->cmd_buf, 0x69, 512);
108memset(i1480->evt_buf, 0x69, 512);
109
110BUG_ON(size > (i1480->buf_size - sizeof(*reply)) / 3);
111result = -ENOMEM;
112cmd->rccb.bCommandType = i1480_CET_VS1;
113cmd->rccb.wCommand = cpu_to_le16(i1480_CMD_MPI_READ);
114cmd->size = cpu_to_le16(3*size);
115for (cnt = 0; cnt < size; cnt++) {
116cmd->data[cnt].page = (srcaddr + cnt) >> 8;
117cmd->data[cnt].offset = (srcaddr + cnt) & 0xff;
118}
119reply->rceb.bEventType = i1480_CET_VS1;
120reply->rceb.wEvent = i1480_CMD_MPI_READ;
121result = i1480_cmd(i1480, "MPI-READ", sizeof(*cmd) + 2*size,
122sizeof(*reply) + 3*size);
123if (result < 0)
124goto out;
125if (reply->bResultCode != UWB_RC_RES_SUCCESS) {
126dev_err(i1480->dev, "MPI-READ: command execution failed:
%d\n",
127reply->bResultCode);
128result = -EIO;
129}
130for (cnt = 0; cnt < size; cnt++) {
131if (reply->data[cnt].page != (srcaddr + cnt) >> 8)
132dev_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);
135if (reply->data[cnt].offset != ((srcaddr + cnt)  
& 0x00ff))

136dev_err(i1480->dev, "MPI-READ: offset
inconsistency at "
137"index %u: expected 0x%02x, got
0x%02x\n", cnt,
138(srcaddr + cnt) & 0x00ff,
139reply->data[cnt].offset);
140data[cnt] = reply->data[cnt].value;
141}
142result = 0;
143out:
144return 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






--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [uwb-i1480] question about value overwrite

[Greg KH]

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{
> 102int result;
> 103struct i1480_cmd_mpi_read *cmd = i1480->cmd_buf;
> 104struct i1480_evt_mpi_read *reply = i1480->evt_buf;
> 105unsigned cnt;
> 106
> 107memset(i1480->cmd_buf, 0x69, 512);
> 108memset(i1480->evt_buf, 0x69, 512);
> 109
> 110BUG_ON(size > (i1480->buf_size - sizeof(*reply)) / 3);
> 111result = -ENOMEM;
> 112cmd->rccb.bCommandType = i1480_CET_VS1;
> 113cmd->rccb.wCommand = cpu_to_le16(i1480_CMD_MPI_READ);
> 114cmd->size = cpu_to_le16(3*size);
> 115for (cnt = 0; cnt < size; cnt++) {
> 116cmd->data[cnt].page = (srcaddr + cnt) >> 8;
> 117cmd->data[cnt].offset = (srcaddr + cnt) & 0xff;
> 118}
> 119reply->rceb.bEventType = i1480_CET_VS1;
> 120reply->rceb.wEvent = i1480_CMD_MPI_READ;
> 121result = i1480_cmd(i1480, "MPI-READ", sizeof(*cmd) + 2*size,
> 122sizeof(*reply) + 3*size);
> 123if (result < 0)
> 124goto out;
> 125if (reply->bResultCode != UWB_RC_RES_SUCCESS) {
> 126dev_err(i1480->dev, "MPI-READ: command execution failed:
> %d\n",
> 127reply->bResultCode);
> 128result = -EIO;
> 129}
> 130for (cnt = 0; cnt < size; cnt++) {
> 131if (reply->data[cnt].page != (srcaddr + cnt) >> 8)
> 132dev_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);
> 135if (reply->data[cnt].offset != ((srcaddr + cnt) & 0x00ff))
> 136dev_err(i1480->dev, "MPI-READ: offset
> inconsistency at "
> 137"index %u: expected 0x%02x, got
> 0x%02x\n", cnt,
> 138(srcaddr + cnt) & 0x00ff,
> 139reply->data[cnt].offset);
> 140data[cnt] = reply->data[cnt].value;
> 141}
> 142result = 0;
> 143out:
> 144return 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html