* [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd()
@ 2013-04-02 7:51 Dan Carpenter
2013-04-17 12:03 ` Dan Carpenter
2014-04-01 14:44 ` [patch] " Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2013-04-02 7:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Antti Palosaari, Michael Krufky, Peter Senna Tschudin,
linux-media, kernel-janitors
I'd like to send this patch except that it "breaks"
cx24116_send_diseqc_msg(). The cx24116 driver accepts ->msg_len values
up to 24 but it looks like it's just copying 16 bytes past the end of
the ->msg[] array so it's already broken.
cmd->msg_len is an unsigned char. The comment next to the struct
declaration says that valid values are are 3-6. Some of the drivers
check that this is true, but most don't and it could cause memory
corruption.
Some examples of functions which don't check are:
ttusbdecfe_dvbs_diseqc_send_master_cmd()
cx24123_send_diseqc_msg()
ds3000_send_diseqc_msg()
etc.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 57601c0..3d1eee6 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2265,7 +2265,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
case FE_DISEQC_SEND_MASTER_CMD:
if (fe->ops.diseqc_send_master_cmd) {
- err = fe->ops.diseqc_send_master_cmd(fe, (struct dvb_diseqc_master_cmd*) parg);
+ struct dvb_diseqc_master_cmd *cmd = parg;
+
+ if (cmd->msg_len >= 3 && cmd->msg_len <= 6)
+ err = fe->ops.diseqc_send_master_cmd(fe, cmd);
+ else
+ err = -EINVAL;
+
fepriv->state = FESTATE_DISEQC;
fepriv->status = 0;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd()
2013-04-02 7:51 [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd() Dan Carpenter
@ 2013-04-17 12:03 ` Dan Carpenter
2013-04-17 19:18 ` Antti Palosaari
2014-04-01 14:44 ` [patch] " Dan Carpenter
1 sibling, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2013-04-17 12:03 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Steven Toth
Cc: Antti Palosaari, Michael Krufky, Peter Senna Tschudin,
Darron Broad, linux-media, kernel-janitors
Any feedback on this?
I forgot to CC Steven Toth last time because he would know about the
cx24116 driver. I've looked at it again and it still looks like
cx24116_send_diseqc_msg() is copying garbage into the
state->dsec_cmd.args[] array.
regards,
dan carpenter
On Tue, Apr 02, 2013 at 10:51:02AM +0300, Dan Carpenter wrote:
> I'd like to send this patch except that it "breaks"
> cx24116_send_diseqc_msg(). The cx24116 driver accepts ->msg_len values
> up to 24 but it looks like it's just copying 16 bytes past the end of
> the ->msg[] array so it's already broken.
>
> cmd->msg_len is an unsigned char. The comment next to the struct
> declaration says that valid values are are 3-6. Some of the drivers
> check that this is true, but most don't and it could cause memory
> corruption.
>
> Some examples of functions which don't check are:
> ttusbdecfe_dvbs_diseqc_send_master_cmd()
> cx24123_send_diseqc_msg()
> ds3000_send_diseqc_msg()
> etc.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 57601c0..3d1eee6 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2265,7 +2265,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>
> case FE_DISEQC_SEND_MASTER_CMD:
> if (fe->ops.diseqc_send_master_cmd) {
> - err = fe->ops.diseqc_send_master_cmd(fe, (struct dvb_diseqc_master_cmd*) parg);
> + struct dvb_diseqc_master_cmd *cmd = parg;
> +
> + if (cmd->msg_len >= 3 && cmd->msg_len <= 6)
> + err = fe->ops.diseqc_send_master_cmd(fe, cmd);
> + else
> + err = -EINVAL;
> +
> fepriv->state = FESTATE_DISEQC;
> fepriv->status = 0;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd()
2013-04-17 12:03 ` Dan Carpenter
@ 2013-04-17 19:18 ` Antti Palosaari
0 siblings, 0 replies; 4+ messages in thread
From: Antti Palosaari @ 2013-04-17 19:18 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Steven Toth, Michael Krufky,
Peter Senna Tschudin, Darron Broad, linux-media, kernel-janitors
On 04/17/2013 03:03 PM, Dan Carpenter wrote:
> Any feedback on this?
>
> I forgot to CC Steven Toth last time because he would know about the
> cx24116 driver. I've looked at it again and it still looks like
> cx24116_send_diseqc_msg() is copying garbage into the
> state->dsec_cmd.args[] array.
It looks fine. I have thought that same few times earlier too. Thank you.
Reviewed-by: Antti Palosaari <crope@iki.fi>
>
> regards,
> dan carpenter
>
> On Tue, Apr 02, 2013 at 10:51:02AM +0300, Dan Carpenter wrote:
>> I'd like to send this patch except that it "breaks"
>> cx24116_send_diseqc_msg(). The cx24116 driver accepts ->msg_len values
>> up to 24 but it looks like it's just copying 16 bytes past the end of
>> the ->msg[] array so it's already broken.
>>
>> cmd->msg_len is an unsigned char. The comment next to the struct
>> declaration says that valid values are are 3-6. Some of the drivers
>> check that this is true, but most don't and it could cause memory
>> corruption.
>>
>> Some examples of functions which don't check are:
>> ttusbdecfe_dvbs_diseqc_send_master_cmd()
>> cx24123_send_diseqc_msg()
>> ds3000_send_diseqc_msg()
>> etc.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
>> index 57601c0..3d1eee6 100644
>> --- a/drivers/media/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb-core/dvb_frontend.c
>> @@ -2265,7 +2265,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>>
>> case FE_DISEQC_SEND_MASTER_CMD:
>> if (fe->ops.diseqc_send_master_cmd) {
>> - err = fe->ops.diseqc_send_master_cmd(fe, (struct dvb_diseqc_master_cmd*) parg);
>> + struct dvb_diseqc_master_cmd *cmd = parg;
>> +
>> + if (cmd->msg_len >= 3 && cmd->msg_len <= 6)
>> + err = fe->ops.diseqc_send_master_cmd(fe, cmd);
>> + else
>> + err = -EINVAL;
>> +
>> fepriv->state = FESTATE_DISEQC;
>> fepriv->status = 0;
>> }
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd()
2013-04-02 7:51 [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd() Dan Carpenter
2013-04-17 12:03 ` Dan Carpenter
@ 2014-04-01 14:44 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2014-04-01 14:44 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Antti Palosaari, Michael Krufky, Peter Senna Tschudin,
linux-media, kernel-janitors
Oops. I send this to Mauro's old email address. Sorry about that.
regards,
dan carpenter
On Tue, Apr 01, 2014 at 05:38:07PM +0300, Dan Carpenter wrote:
> I'd like to send this patch except that it "breaks"
> cx24116_send_diseqc_msg(). The cx24116 driver accepts ->msg_len values
> up to 24 but it looks like it's just copying 16 bytes past the end of
> the ->msg[] array so it's already broken.
>
> cmd->msg_len is an unsigned char. The comment next to the struct
> declaration says that valid values are are 3-6. Some of the drivers
> check that this is true, but most don't and it could cause memory
> corruption.
>
> Some examples of functions which don't check are:
> ttusbdecfe_dvbs_diseqc_send_master_cmd()
> cx24123_send_diseqc_msg()
> ds3000_send_diseqc_msg()
> etc.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Antti Palosaari <crope@iki.fi>
> ---
> This is a static checker fix and I haven't tested it but the security
> implications are quite bad so we should fix this.
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 57601c0..3d1eee6 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2267,7 +2267,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file,
>
> case FE_DISEQC_SEND_MASTER_CMD:
> if (fe->ops.diseqc_send_master_cmd) {
> - err = fe->ops.diseqc_send_master_cmd(fe, (struct dvb_diseqc_master_cmd*) parg);
> + struct dvb_diseqc_master_cmd *cmd = parg;
> +
> + if (cmd->msg_len >= 3 && cmd->msg_len <= 6)
> + err = fe->ops.diseqc_send_master_cmd(fe, cmd);
> + else
> + err = -EINVAL;
> +
> fepriv->state = FESTATE_DISEQC;
> fepriv->status = 0;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-01 14:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-02 7:51 [RFC] [media] dvb-core: check ->msg_len for diseqc_send_master_cmd() Dan Carpenter
2013-04-17 12:03 ` Dan Carpenter
2013-04-17 19:18 ` Antti Palosaari
2014-04-01 14:44 ` [patch] " Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).