| Message ID | 20130402075102.GA11233@longonot.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; >> }
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; > } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html