Skip to contentSkip to conversation

question about value overwrite

uwb-i1480 New Gustavo A. R. Silva 2 participants 4 messages May 18, 2017 – May 19, 2017

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

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

drivers/uwb/i1480/dfu/phy.c +0 -0
@@ -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

#1 lore raw
Quoted 81 lines · Gustavo A. R. Silva

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

drivers/uwb/i1480/dfu/phy.c +1 -0
@@ -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)
Quoted 3 lines

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

author
#2 lore raw

Hi Greg,

Quoting Greg KH <[email protected]>:

Quoted 82 lines · Gustavo A. R. Silva
Quoted 81 lines

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

drivers/uwb/i1480/dfu/phy.c +1 -0
@@ -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)
Quoted 7 lines
Quoted 3 lines

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

author
#3 lore raw

PATCH 1/1 uwb: i1480: add missing goto

Add missing goto.

Addresses-Coverity-ID: 1226913

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/uwb/i1480/dfu/phy.c | 1 +
 1 file changed, 1 insertion(+)
drivers/uwb/i1480/dfu/phy.c +1 -0
index 3b1a87d..1ac8526 100644
@@ -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)
Signature · 1 line
2.5.0