Re: [uwb-i1480] question about value overwrite

[Gustavo A. R. Silva]

Hi Greg,

Quoting Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:

> 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