Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1644945 > unrolled thread
| Started by | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| First post | 2017-05-19 01:10 +0200 |
| Last post | 2017-05-19 10:30 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[uwb-i1480] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-19 01:10 +0200
Re: [uwb-i1480] question about value overwrite Greg KH <gregkh@linuxfoundation.org> - 2017-05-19 07:40 +0200
Re: [uwb-i1480] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-19 10:20 +0200
[PATCH] uwb: i1480: add missing goto "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-19 10:30 +0200
| From | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| Date | 2017-05-19 01:10 +0200 |
| Subject | [uwb-i1480] question about value overwrite |
| Message-ID | <tICIp-1Ed-3@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Greg KH <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-05-19 07:40 +0200 |
| Message-ID | <tIINP-5PI-3@gated-at.bofh.it> |
| In reply to | #1644945 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| Date | 2017-05-19 10:20 +0200 |
| Message-ID | <tILiH-7O7-57@gated-at.bofh.it> |
| In reply to | #1645208 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| Date | 2017-05-19 10:30 +0200 |
| Subject | [PATCH] uwb: i1480: add missing goto |
| Message-ID | <tILsm-7S6-35@gated-at.bofh.it> |
| In reply to | #1645335 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web