Messages in this thread | | | Date | Fri, 19 May 2017 03:13:58 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [uwb-i1480] question about value overwrite |
| |
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
| |