Hi,
On Thu, Jul 29, 2021 at 12:32:21PM +0200, Greg KH wrote:
> On Wed, Apr 14, 2021 at 11:57:59AM +0300, Dan Carpenter wrote:
> > The bounds checking in avc_ca_pmt() is not strict enough. It should
> > be checking "read_pos + 4" because it's reading 5 bytes. If the
> > "es_info_length" is non-zero then it reads a 6th byte so there needs to
> > be an additional check for that.
> >
> > I also added checks for the "write_pos". I don't think these are
> > required because "read_pos" and "write_pos" are tied together so
> > checking one ought to be enough. But they make the code easier to
> > understand for me.
> >
> > The other problem is that "length" can be invalid. It comes from
> > "data_length" in fdtv_ca_pmt().
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Luo Likang <luolikang@xxxxxxxxxxx>
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > This hardware isn't super common so there is no embargo. Resending
> > through normal lists.
> >
> > Oh, another thing is the data_length calculation in fdtv_ca_pmt() seems
> > very suspicous. Reading more than 4 bytes in the loop will lead to
> > shift wrapping.
> >
> > drivers/media/firewire/firedtv-avc.c | 14 +++++++++++---
> > drivers/media/firewire/firedtv-ci.c | 2 ++
> > 2 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/firewire/firedtv-avc.c b/drivers/media/firewire/firedtv-avc.c
> > index 2bf9467b917d..71991f8638e6 100644
> > --- a/drivers/media/firewire/firedtv-avc.c
> > +++ b/drivers/media/firewire/firedtv-avc.c
> > @@ -1165,7 +1165,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++];
> > @@ -1177,13 +1181,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 9363d005e2b6..2d6992ac5dd6 100644
> > --- a/drivers/media/firewire/firedtv-ci.c
> > +++ b/drivers/media/firewire/firedtv-ci.c
> > @@ -134,6 +134,8 @@ static int fdtv_ca_pmt(struct firedtv *fdtv, void *arg)
> > } else {
> > data_length = msg->msg[3];
> > }
> > + if (data_length > sizeof(msg->msg) - 4)
> > + return -EINVAL;
> >
> > return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length);
> > }
> > --
> > 2.30.2
> >
>
> This patch seems to have gotten lost. Any change of it getting applied?
As far I can see there was then a version 2 of the patch, but that one
got list somewhere. Friendly ping on this thread :)
Regards,
Salvatore
