Re: [media] tda10071: NXP TDA10071 DVB-S/S2 driver

[Antti Palosaari <crope@iki.fi>]

Moikka Dan,
and thank you. Comments below.

On 17.04.2012 13:33, Dan Carpenter wrote:
> Hi Antti,
>
> Smatch complains about a potential information leak.  I was hoping you
> could take a look.
>
> The patch de8e42035014: "[media] tda10071: NXP TDA10071 DVB-S/S2
> driver" from Aug 1, 2011, leads to the following warning:
> drivers/media/dvb/frontends/tda10071.c:322
> tda10071_diseqc_send_master_cmd()
> 	 error: memcpy() 'diseqc_cmd->msg' too small (6 vs 16)
>
>
> drivers/media/dvb/frontends/tda10071.c
>     290          if (diseqc_cmd->msg_len<  3 || diseqc_cmd->msg_len>  16) {
>                                                 ^^^^^^^^^^^^^^^^^^^^^^^^
> We cap ->msg_len at 16 here.  I wasn't able to figure out where the 16
> came from.  Or the 3 for that matter.

Those numbers are coming from include/linux/dvb/frontend.h struct 
dvb_diseqc_master_cmd. And initially values are from the DiSEqC spec.
But you are correct, it is bug. Upper limit for message len should be 6 
instead of 16 used currently. Likely just typo. Maybe I have done some 
len testing during the development and changed it temporarily 6 => 16 
and forgot switch back. Who knows...

>
>     291                  ret = -EINVAL;
>     292                  goto error;
>     293          }
>     294
>     295          /* wait LNB TX */
>     296          for (i = 500, tmp = 0; i&&  !tmp; i--) {
>     297                  ret = tda10071_rd_reg_mask(priv, 0x47,&tmp, 0x01);
>     298                  if (ret)
>     299                          goto error;
>     300
>     301                  usleep_range(10000, 20000);
>     302          }
>     303
>     304          dbg("%s: loop=%d", __func__, i);
>     305
>     306          if (i == 0) {
>     307                  ret = -ETIMEDOUT;
>     308                  goto error;
>     309          }
>     310
>     311          ret = tda10071_wr_reg_mask(priv, 0x47, 0x00, 0x01);
>     312          if (ret)
>     313                  goto error;
>     314
>     315          cmd.args[0x00] = CMD_LNB_SEND_DISEQC;
>     316          cmd.args[0x01] = 0;
>     317          cmd.args[0x02] = 0;
>     318          cmd.args[0x03] = 0;
>     319          cmd.args[0x04] = 2;
>     320          cmd.args[0x05] = 0;
>     321          cmd.args[0x06] = diseqc_cmd->msg_len;
>     322          memcpy(&cmd.args[0x07], diseqc_cmd->msg, diseqc_cmd->msg_len);
>                                          ^^^^^^^^^^^^^^^
> ->msg is only 6 bytes long so we're copying past the end of the array.
>
> Also cmd.arg is 0x1e (30) bytes long and we only copy 0x07 + 16 bytes
> into it so it leaves the last 7 bytes of cmd.args unitialized.  Btw,
> why are the sizes specified in hex instead of decimal here?
>
>     323          cmd.len = 0x07 + diseqc_cmd->msg_len;

What it happens now is that garbage data will be send to DiSEqC switch - 
in case of garbage data is sent from the user-space. Anyhow, I would 
like to rather move these kind of common validly checking to the 
DVB-core. Not only that case, but for the more commonly too. IMHO there 
is currently too general checking left for the individual drivers...

And for the usage of hex numbering - I don't remember. Overall I prefer 
hex numbering unless values are not clearly decimal ones, like 
frequencies. But I agree indexing like that is seems more readable when 
decimal numbering is used. I have generally used decimal numbering in 
such cases.

Feel free to sent patch - or I will fix it someday later when suitable 
time is found.

thanks,
Antti
-
http://palosaari.fi/