Skip to contentSkip to conversation

dvb-core: check ->msg_len for diseqc_send_master_cmd()

PATCH In review Dan Carpenter 2 participants 4 messages Apr 2, 2013 – Apr 1, 2014

Subsystems (2)
Media (V4L/DVB) Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dvb-core: check ->msg_len for diseqc_send_master_cmd()

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 <[email protected]>
drivers/media/dvb-core/dvb_frontend.c +7 -1
index 57601c0..3d1eee6 100644
@@ -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;
 		}
author
#1 lore raw

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

Quoted 37 lines · Dan Carpenter

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 <[email protected]>
drivers/media/dvb-core/dvb_frontend.c +7 -1
index 57601c0..3d1eee6 100644
@@ -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;
 		}
Reviewed-by
#2 lore raw
Quoted 6 lines · Dan Carpenter

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 <[email protected]>
Quoted 42 lines

regards,
dan carpenter

Quoted 37 lines · Dan Carpenter

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 <[email protected]>
drivers/media/dvb-core/dvb_frontend.c +7 -1
index 57601c0..3d1eee6 100644
@@ -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;
  		}
Signature · 1 line
http://palosaari.fi/
author
#3 lore raw
In reply to Dan Carpenter

Oops. I send this to Mauro's old email address. Sorry about that.

regards,
dan carpenter

Quoted 41 lines · Dan Carpenter

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 <[email protected]>
Reviewed-by Antti Palosaari <[email protected]>

---
This is a static checker fix and I haven't tested it but the security implications are quite bad so we should fix this.

drivers/media/dvb-core/dvb_frontend.c +7 -1
index 57601c0..3d1eee6 100644
@@ -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;
 		}