| Message ID | 20210607073900.1298-1-yangyanchao6@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media firewire firedtv-avc fix a buffer overflow in avc_ca_pmt() | expand |
On Mon, Jun 07, 2021 at 03:39:00PM +0800, Yang Yanchao wrote: > For CVE-2021-3542: What does that mean? We don't know what cve numbers refer to as there is no way to really track and update the information with them. Please spell out the issue please. > > 1???read_pos will be added four times in the patch, > so use "read_pos + 4 < length" and write_pos as well what is "???" here? > > 2. The last four bits of c->operand are used for CRC, > so "sizeof (C - > operand) - 4" is used > > 3. "read_pos+=2" is added after the end of read_pos, so add value (read_pos >= length) > > 4. In order to avoid memcpy crossing the boundary, es_ info_ length > length - read_ pos > > 5. When the date_length is a specific input of a construction,it will cause memcpy > to exceed the boundary, "(MSG - > MSG [3] & 0x7F) + date_ length) > (sizeof(msg->msg) - 4)" I do not understand, this is saying what you did, not _why_ you did it. can you please rework this to make it more obvious what you are doing? And shouldn't this be more than one patch? A series of patches, each fixing one thing? And no need to put security@kernel.org on this now that you have sent it to a public mailing list. thanks, greg k-h
Thanks for resending this patch, but you need to preserve the author and Reported-by tags. https://lore.kernel.org/linux-media/YHaulytonFcW+lyZ@mwanda/ You changed the check in fdtv_ca_pmt() but I don't understand why you did that... But looking at it again, I think neither of us was correct, instead of "sizeof(msg->msg) - 4" it should be "- data_pos": if (data_length > sizeof(msg->msg) - data_pos) return -EINVAL; I will resend a v2. regards, dan carpenter
diff --git a/drivers/media/firewire/firedtv-avc.c b/drivers/media/firewire/firedtv-avc.c index 3ef5df164..8c31cf90c 100644 --- a/drivers/media/firewire/firedtv-avc.c +++ b/drivers/media/firewire/firedtv-avc.c @@ -1169,7 +1169,11 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length) read_pos += program_info_length; write_pos += program_info_length; } - while (read_pos < length) { + while (read_pos + 4 < length) { + if (write_pos + 4 >= sizeof(c->operand) - 4) { + ret = -EINVAL; + goto out; + } c->operand[write_pos++] = msg[read_pos++]; c->operand[write_pos++] = msg[read_pos++]; c->operand[write_pos++] = msg[read_pos++]; @@ -1181,13 +1185,17 @@ int avc_ca_pmt(struct firedtv *fdtv, char *msg, int length) c->operand[write_pos++] = es_info_length >> 8; c->operand[write_pos++] = es_info_length & 0xff; if (es_info_length > 0) { + if (read_pos >= length) { + ret = -EINVAL; + goto out; + } pmt_cmd_id = msg[read_pos++]; if (pmt_cmd_id != 1 && pmt_cmd_id != 4) 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) { + if (es_info_length > sizeof(c->operand) - 4 - write_pos || + es_info_length > length - read_pos) { ret = -EINVAL; goto out; } diff --git a/drivers/media/firewire/firedtv-ci.c b/drivers/media/firewire/firedtv-ci.c index 8dc5a7495..0e7ffa156 100644 --- a/drivers/media/firewire/firedtv-ci.c +++ b/drivers/media/firewire/firedtv-ci.c @@ -135,6 +135,8 @@ static int fdtv_ca_pmt(struct firedtv *fdtv, void *arg) data_length = 0; for (i = 0; i < (msg->msg[3] & 0x7f); i++) data_length = (data_length << 8) + msg->msg[data_pos++]; + if (((msg->msg[3] & 0x7f) + date_length) > (sizeof(msg->msg) - 4)) + return -EINVAL; } else { data_length = msg->msg[3]; }
For CVE-2021-3542: 1、read_pos will be added four times in the patch, so use "read_pos + 4 < length" and write_pos as well 2. The last four bits of c->operand are used for CRC, so "sizeof (C - > operand) - 4" is used 3. "read_pos+=2" is added after the end of read_pos, so add value (read_pos >= length) 4. In order to avoid memcpy crossing the boundary, es_ info_ length > length - read_ pos 5. When the date_length is a specific input of a construction,it will cause memcpy to exceed the boundary, "(MSG - > MSG [3] & 0x7F) + date_ length) > (sizeof(msg->msg) - 4)" Signed-off-by: yangyanchao <yangyanchao6@huawei.com> --- drivers/media/firewire/firedtv-avc.c | 14 +++++++++++--- drivers/media/firewire/firedtv-ci.c | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-)