* [uwb-i1480] question about value overwrite
@ 2017-05-18 23:00 Gustavo A. R. Silva
2017-05-19 5:33 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-18 23:00 UTC (permalink / raw)
To: linux-usb; +Cc: linux-kernel
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.
Thank you!
--
Gustavo A. R. Silva
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [uwb-i1480] question about value overwrite
2017-05-18 23:00 [uwb-i1480] question about value overwrite Gustavo A. R. Silva
@ 2017-05-19 5:33 ` Greg KH
2017-05-19 8:13 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2017-05-19 5:33 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [uwb-i1480] question about value overwrite
2017-05-19 5:33 ` Greg KH
@ 2017-05-19 8:13 ` Gustavo A. R. Silva
2017-05-19 8:22 ` [PATCH] uwb: i1480: add missing goto Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-19 8:13 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] uwb: i1480: add missing goto
2017-05-19 8:13 ` Gustavo A. R. Silva
@ 2017-05-19 8:22 ` Gustavo A. R. Silva
0 siblings, 0 replies; 4+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-19 8:22 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Gustavo A. R. Silva
Add missing goto.
Addresses-Coverity-ID: 1226913
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/uwb/i1480/dfu/phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/uwb/i1480/dfu/phy.c b/drivers/uwb/i1480/dfu/phy.c
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)
--
2.5.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-19 8:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-18 23:00 [uwb-i1480] question about value overwrite Gustavo A. R. Silva
2017-05-19 5:33 ` Greg KH
2017-05-19 8:13 ` Gustavo A. R. Silva
2017-05-19 8:22 ` [PATCH] uwb: i1480: add missing goto Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox