linux-kernel - [uwb-i1480] question about value overwrite

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 May 2017 18:00:06 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     linux-usb@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: [uwb-i1480] question about value overwrite


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