Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644918

Re: [media-dvb-usb-v2] question about value overwrite

Show all headers | View raw

Hi Malcolm,

Quoting Malcolm Priestley <tvboxspy@gmail.com>:

> Hi
>
> On 18/05/17 20:09, Gustavo A. R. Silva wrote:
>>
>> Hello everybody,
>>
>> While looking into Coverity ID 1226934 I ran into the following  
>> piece of code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205
>>
>> 205static int lme2510_stream_restart(struct dvb_usb_device *d)
>> 206{
>> 207        struct lme2510_state *st = d->priv;
>> 208        u8 all_pids[] = LME_ALL_PIDS;
>> 209        u8 stream_on[] = LME_ST_ON_W;
>> 210        int ret;
>> 211        u8 rbuff[1];
>> 212        if (st->pid_off)
>> 213                ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
>> 214                        rbuff, sizeof(rbuff));
>> 215        /*Restart Stream Command*/
>> 216        ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
>> 217                        rbuff, sizeof(rbuff));
>> 218        return ret;
>> 219}
>
> It is a mistake it should have been ORed ad in |= as  
> lme2510_usb_talk only returns three states.
>

I see now. The idea is to code something similar to the following  
piece of code in the same file:

242
243        ret |= lme2510_usb_talk(d, pid_buff ,
244                sizeof(pid_buff) , rbuf, sizeof(rbuf));
245
246        if (st->stream_on)
247                ret |= lme2510_stream_restart(d);
248
249        return ret;

right?

So in this case, the following patch would properly fix the bug:

index 924adfd..3ab1754 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,13 +207,14 @@ static int lme2510_stream_restart(struct  
dvb_usb_device *d)
         struct lme2510_state *st = d->priv;
         u8 all_pids[] = LME_ALL_PIDS;
         u8 stream_on[] = LME_ST_ON_W;
-       int ret;
+       int ret = 0;
         u8 rbuff[1];
+
         if (st->pid_off)
                 ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
                         rbuff, sizeof(rbuff));
         /*Restart Stream Command*/
-       ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+       ret |= lme2510_usb_talk(d, stream_on, sizeof(stream_on),
                         rbuff, sizeof(rbuff));
         return ret;
  }

What do you think?

> So if an error is in the running it will be returned to user.
>
> The first of your patches is better and more or less the same, the  
> second would break driver, restart is not an else condition.
>

Thank you for the clarification.
--
Gustavo A. R. Silva

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[media-dvb-usb-v2] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-18 21:10 +0200
  Re: [media-dvb-usb-v2] question about value overwrite Malcolm Priestley <tvboxspy@gmail.com> - 2017-05-18 22:00 +0200
    Re: [media-dvb-usb-v2] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-18 22:40 +0200

csiph-web