firewire: firedtv-avc: potential buffer overflow
Subsystems (3)
PATCH 1/1 firewire: firedtv-avc: potential buffer overflow
"program_info_length" is user controlled and can go up to 4095. The operand[] array has 509 bytes so we need to add a limit here to prevent buffer overflows.
drivers/media/firewire/firedtv-avc.c +9 -0
@@ -1157,6 +1157,10 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)if (pmt_cmd_id !=1 && pmt_cmd_id !=4 )dev_err (fdtv->device,"invalid pmt_cmd_id %d\n" , pmt_cmd_id); +if (program_info_length >sizeof (c->operand) - write_pos) { + ret = -EINVAL ; + goto out; + }memcpy (&c->operand[write_pos], &msg[read_pos], program_info_length);
@@ -1180,6 +1184,11 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)dev_err (fdtv->device,"invalid pmt_cmd_id %d " "at stream level\n" , pmt_cmd_id); +if (es_info_length >sizeof (c->operand) - write_pos) { + ret = -EINVAL ; + goto out; + } +memcpy (&c->operand[write_pos], &msg[read_pos], es_info_length); read_pos += es_info_length;
Quoted 5 lines · On Sep 08 Dan Carpenter wrote:
"
program_info_length" is user controlled and can go up to 4095. The operand[] array has 509 bytes so we need to add a limit here to prevent buffer overflows.Signed-off-by Dan Carpenter <[email protected]>
Thank you.
Quoted 28 lines
drivers/media/firewire/firedtv-avc.c +9
-0@@ -1157,6 +1157,10 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)if (pmt_cmd_id !=1 && pmt_cmd_id !=4 )dev_err (fdtv->device,"invalid pmt_cmd_id %d\n" , pmt_cmd_id); +if (program_info_length >sizeof (c->operand) - write_pos) { + ret = -EINVAL ; + goto out; + }memcpy (&c->operand[write_pos], &msg[read_pos], program_info_length);@@ -1180,6 +1184,11 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)dev_err (fdtv->device,"invalid pmt_cmd_id %d " "at stream level\n" , pmt_cmd_id); +if (es_info_length >sizeof (c->operand) - write_pos) { + ret = -EINVAL ; + goto out; + } +memcpy (&c->operand[write_pos], &msg[read_pos], es_info_length); read_pos += es_info_length;
Signature · 3 lines
Stefan Richter -===-==- =--= -=--- http://arcgraph.de/sr/
Stefan Richter
maintainerQuoted 10 lines · On Sep 08 Stefan Richter wrote:
Quoted 5 lines · On Sep 08 Dan Carpenter wrote:
"
program_info_length" is user controlled and can go up to 4095. The operand[] array has 509 bytes so we need to add a limit here to prevent buffer overflows.Signed-off-by Dan Carpenter <[email protected]>Reviewed-by Stefan Richter <[email protected]>Thank you.
Oops, that was a bit too quick. After the memcpy() accesses which you protect, there are another four bytes written, still without checking the bounds.
Quoted 10 lines
drivers/media/firewire/firedtv-avc.c int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length) · line 1160
Quoted 10 lines
drivers/media/firewire/firedtv-avc.c +1
-0@@ -1157,6 +1157,10 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)if (pmt_cmd_id !=1 && pmt_cmd_id !=4 )dev_err (fdtv->device,"invalid pmt_cmd_id %d\n" , pmt_cmd_id); +if (program_info_length >sizeof (c->operand) - write_pos) {
So I suggest something like this instead:
+ if (program_info_length > sizeof(c->operand) - 4 - write_pos) {
Quoted 11 lines
drivers/media/firewire/firedtv-avc.c int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length) · line 1187
Quoted 11 lines
drivers/media/firewire/firedtv-avc.c +4
-0+ ret = -EINVAL ; + goto out; + }memcpy (&c->operand[write_pos], &msg[read_pos], program_info_length);@@ -1180,6 +1184,11 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)dev_err (fdtv->device,"invalid pmt_cmd_id %d " "at stream level\n" , pmt_cmd_id); +if (es_info_length >sizeof (c->operand) - write_pos) {
And likewise:
+ if (es_info_length > sizeof(c->operand) - 4 - write_pos) {
Quoted 7 lines
drivers/media/firewire/firedtv-avc.c int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length) · line 1194
Quoted 7 lines
drivers/media/firewire/firedtv-avc.c +4
-0+ ret = -EINVAL ; + goto out; + } +memcpy (&c->operand[write_pos], &msg[read_pos], es_info_length); read_pos += es_info_length;
FYI, after this follows:
write_pos += es_info_length; } } write_pos += 4; /* CRC */ c->operand[7] = 0x82; c->operand[8] = (write_pos - 10) >> 8; c->operand[9] = (write_pos - 10) & 0xff; c->operand[14] = write_pos - 15; crc32_csum = crc32_be(0, &c->operand[10], c->operand[12] - 1); c->operand[write_pos - 4] = (crc32_csum >> 24) & 0xff; c->operand[write_pos - 3] = (crc32_csum >> 16) & 0xff; c->operand[write_pos - 2] = (crc32_csum >> 8) & 0xff; c->operand[write_pos - 1] = (crc32_csum >> 0) & 0xff; pad_operands(c, write_pos); fdtv->avc_data_length = ALIGN(3 + write_pos, 4); ret = avc_write(fdtv);
And pad_operands() is defined in the same source file as:
#define LAST_OPERAND (509 - 1)
static inline void clear_operands(struct avc_command_frame *c, int from, int to)
{
memset(&c->operand[from], 0, to - from + 1);
}static void pad_operands(struct avc_command_frame *c, int from)
{
int to = ALIGN(from, 4);
if (from <= to && to <= LAST_OPERAND)
clear_operands(c, from, to);
}BTW, the calculation of "to" in pad_operands appears to be wrong, but this does not affect Dan's patch. I will send an extra patch for that.
Regards,
Signature · 3 lines
Stefan Richter -===-==- =--= -=--- http://arcgraph.de/sr/
Quoted 8 lines · Stefan Richter
Quoted 7 lines · On Sep 08 Stefan Richter wrote:
Quoted 5 lines · On Sep 08 Dan Carpenter wrote:
"
program_info_length" is user controlled and can go up to 4095. The operand[] array has 509 bytes so we need to add a limit here to prevent buffer overflows.Signed-off-by Dan Carpenter <[email protected]>
Quoted 7 lines
Quoted 3 lines
Reviewed-by Stefan Richter <[email protected]>Thank you.
Oops, that was a bit too quick. After the
memcpy()accesses which you protect, there are another four bytes written, still without checking the bounds.
Thanks for catching that. I'll send a v2 soon.
Btw, my static checker complains about the remaining memcpy() in this file:
drivers/media/firewire/firedtv-avc.c:1310 avc_ca_get_mmi() error: '*len' from user is not capped properly
This static checker warning has a lot of false positives. I looked at the code for a long time but couldn't figure out why it thinks "*len" is untrusted. I also wasn't totally sure that it was safe?
regards,
dan carpenter
PATCH v2 1/1 firewire: firedtv-avc: potential buffer overflow
In reply to Stefan Richter"program_info_length" is user controlled and can go up to 4095. The operand[] array has 509 bytes so we need to add a limit here to prevent buffer overflows.
The " - 4" in the limit check is because we have 4 bytes more data to add after the memcpy().
---
v2: The first version didn't have the - 4. Thanks for catching that Stafan.
drivers/media/firewire/firedtv-avc.c +10 -0
@@ -1157,6 +1157,10 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)if (pmt_cmd_id !=1 && pmt_cmd_id !=4 )dev_err (fdtv->device,"invalid pmt_cmd_id %d\n" , pmt_cmd_id); +if (program_info_length >sizeof (c->operand) -4 - write_pos) { + ret = -EINVAL ; + goto out; + }memcpy (&c->operand[write_pos], &msg[read_pos], program_info_length);
@@ -1180,6 +1184,12 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length)dev_err (fdtv->device,"invalid pmt_cmd_id %d " "at stream level\n" , pmt_cmd_id); +if (es_info_length >sizeof (c->operand) -4 - + write_pos) { + ret = -EINVAL ; + goto out; + } +memcpy (&c->operand[write_pos], &msg[read_pos], es_info_length); read_pos += es_info_length;