[PATCH] media: dvb: fix a missing-check bug

[PATCH] media: dvb: fix a missing-check bug

[Wenwen Wang]

In dvb_audio_write(), the first byte of the user-space buffer 'buf' is
firstly copied and checked to see whether this is a TS packet, which always
starts with 0x47 for synchronization purposes. If yes, ts_play() will be
called. Otherwise, dvb_aplay() will be called. In ts_play(), the content of
'buf', including the first byte, is copied again from the user space.
However, after the copy, no check is re-enforced on the first byte of the
copied data.  Given that 'buf' is in the user space, a malicious user can
race to change the first byte after the check in dvb_audio_write() but
before the copy in ts_play(). Through this way, the user can supply
inconsistent code, which can cause undefined behavior of the kernel and
introduce potential security risk.

This patch adds a necessary check in ts_play() to make sure the first byte
acquired in the second copy contains the expected value. Otherwise, an
error code EINVAL will be returned.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/media/pci/ttpci/av7110_av.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/ttpci/av7110_av.c b/drivers/media/pci/ttpci/av7110_av.c
index ef1bc17..1ff6062 100644
--- a/drivers/media/pci/ttpci/av7110_av.c
+++ b/drivers/media/pci/ttpci/av7110_av.c
@@ -468,6 +468,8 @@ static ssize_t ts_play(struct av7110 *av7110, const char __user *buf,
 		}
 		if (copy_from_user(kb, buf, TS_SIZE))
 			return -EFAULT;
+		if (kb[0] != 0x47)
+			return -EINVAL;
 		write_ts_to_decoder(av7110, type, kb, TS_SIZE);
 		todo -= TS_SIZE;
 		buf += TS_SIZE;
-- 
2.7.4

Re: [PATCH] media: dvb: fix a missing-check bug

[Wenwen Wang]

Hello,

Can anyone confirm this bug? Thanks!

Wenwen

On Fri, Oct 19, 2018 at 9:12 AM Wenwen Wang <wang6495@umn.edu> wrote:
>
> In dvb_audio_write(), the first byte of the user-space buffer 'buf' is
> firstly copied and checked to see whether this is a TS packet, which always
> starts with 0x47 for synchronization purposes. If yes, ts_play() will be
> called. Otherwise, dvb_aplay() will be called. In ts_play(), the content of
> 'buf', including the first byte, is copied again from the user space.
> However, after the copy, no check is re-enforced on the first byte of the
> copied data.  Given that 'buf' is in the user space, a malicious user can
> race to change the first byte after the check in dvb_audio_write() but
> before the copy in ts_play(). Through this way, the user can supply
> inconsistent code, which can cause undefined behavior of the kernel and
> introduce potential security risk.
>
> This patch adds a necessary check in ts_play() to make sure the first byte
> acquired in the second copy contains the expected value. Otherwise, an
> error code EINVAL will be returned.
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/media/pci/ttpci/av7110_av.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/pci/ttpci/av7110_av.c b/drivers/media/pci/ttpci/av7110_av.c
> index ef1bc17..1ff6062 100644
> --- a/drivers/media/pci/ttpci/av7110_av.c
> +++ b/drivers/media/pci/ttpci/av7110_av.c
> @@ -468,6 +468,8 @@ static ssize_t ts_play(struct av7110 *av7110, const char __user *buf,
>                 }
>                 if (copy_from_user(kb, buf, TS_SIZE))
>                         return -EFAULT;
> +               if (kb[0] != 0x47)
> +                       return -EINVAL;
>                 write_ts_to_decoder(av7110, type, kb, TS_SIZE);
>                 todo -= TS_SIZE;
>                 buf += TS_SIZE;
> --
> 2.7.4
>

Re: [PATCH] media: dvb: fix a missing-check bug

[Bing Bu Cao]

I think need Cc to Lad, Prabhakar <prabhakar.csengg@gmail.com>

On 10/30/2018 02:46 AM, Wenwen Wang wrote:
> Hello,
>
> Can anyone confirm this bug? Thanks!
>
> Wenwen
>
> On Fri, Oct 19, 2018 at 9:12 AM Wenwen Wang <wang6495@umn.edu> wrote:
>> In dvb_audio_write(), the first byte of the user-space buffer 'buf' is
>> firstly copied and checked to see whether this is a TS packet, which always
>> starts with 0x47 for synchronization purposes. If yes, ts_play() will be
>> called. Otherwise, dvb_aplay() will be called. In ts_play(), the content of
>> 'buf', including the first byte, is copied again from the user space.
>> However, after the copy, no check is re-enforced on the first byte of the
>> copied data.  Given that 'buf' is in the user space, a malicious user can
>> race to change the first byte after the check in dvb_audio_write() but
>> before the copy in ts_play(). Through this way, the user can supply
>> inconsistent code, which can cause undefined behavior of the kernel and
>> introduce potential security risk.
>>
>> This patch adds a necessary check in ts_play() to make sure the first byte
>> acquired in the second copy contains the expected value. Otherwise, an
>> error code EINVAL will be returned.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>>  drivers/media/pci/ttpci/av7110_av.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/pci/ttpci/av7110_av.c b/drivers/media/pci/ttpci/av7110_av.c
>> index ef1bc17..1ff6062 100644
>> --- a/drivers/media/pci/ttpci/av7110_av.c
>> +++ b/drivers/media/pci/ttpci/av7110_av.c
>> @@ -468,6 +468,8 @@ static ssize_t ts_play(struct av7110 *av7110, const char __user *buf,
>>                 }
>>                 if (copy_from_user(kb, buf, TS_SIZE))
>>                         return -EFAULT;
>> +               if (kb[0] != 0x47)
>> +                       return -EINVAL;
>>                 write_ts_to_decoder(av7110, type, kb, TS_SIZE);
>>                 todo -= TS_SIZE;
>>                 buf += TS_SIZE;
>> --
>> 2.7.4
>>

Re: [PATCH] media: dvb: fix a missing-check bug

[Sean Young]

Hi Wenwen,

On Mon, Oct 29, 2018 at 01:46:04PM -0500, Wenwen Wang wrote:
> Hello,
> 
> Can anyone confirm this bug? Thanks!
> 
> Wenwen
> 
> On Fri, Oct 19, 2018 at 9:12 AM Wenwen Wang <wang6495@umn.edu> wrote:
> >
> > In dvb_audio_write(), the first byte of the user-space buffer 'buf' is
> > firstly copied and checked to see whether this is a TS packet, which always
> > starts with 0x47 for synchronization purposes. If yes, ts_play() will be
> > called. Otherwise, dvb_aplay() will be called. In ts_play(), the content of
> > 'buf', including the first byte, is copied again from the user space.
> > However, after the copy, no check is re-enforced on the first byte of the
> > copied data.  Given that 'buf' is in the user space, a malicious user can
> > race to change the first byte after the check in dvb_audio_write() but
> > before the copy in ts_play().

Up to here your analysis makes sense.

> >  Through this way, the user can supply
> > inconsistent code, which can cause undefined behavior of the kernel and
> > introduce potential security risk.

So how can this cause undefined behaviour?

> > This patch adds a necessary check in ts_play() to make sure the first byte
> > acquired in the second copy contains the expected value. Otherwise, an
> > error code EINVAL will be returned.

So what about the other case, if dvb_play() was called due to the first
byte not being 0x47 and then swapped for 0x47?


Sean

> >
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> >  drivers/media/pci/ttpci/av7110_av.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/pci/ttpci/av7110_av.c b/drivers/media/pci/ttpci/av7110_av.c
> > index ef1bc17..1ff6062 100644
> > --- a/drivers/media/pci/ttpci/av7110_av.c
> > +++ b/drivers/media/pci/ttpci/av7110_av.c
> > @@ -468,6 +468,8 @@ static ssize_t ts_play(struct av7110 *av7110, const char __user *buf,
> >                 }
> >                 if (copy_from_user(kb, buf, TS_SIZE))
> >                         return -EFAULT;
> > +               if (kb[0] != 0x47)
> > +                       return -EINVAL;
> >                 write_ts_to_decoder(av7110, type, kb, TS_SIZE);
> >                 todo -= TS_SIZE;
> >                 buf += TS_SIZE;
> > --
> > 2.7.4
> >

[PATCH] media: dvb: fix a missing-check bug

[Wenwen Wang]

In dvb_video_write(), the first header byte of the buffer 'buf' supplied by
the user is checked to see whether 'buf' contains a TS packet, which
always starts with 0x47 for synchronization purposes. If yes, ts_play() is
called. Otherwise, dvb_play() will be called. Both of these two functions
will copy 'buf' again from the user space. However, no check is enforced
on the first byte of the copied content after the second copy. Since 'buf'
is in the user space, a malicious user can race to change the first byte
after the check in dvb_video_write() but before the second copy in
ts_play(). By doing so, the user can supply inconsistent data, which can
lead to undefined behavior in the driver.

This patch adds the required check in ts_play() to make sure the header
byte in the second copy is as expected. Otherwise an error code EINVAL will
be returned.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/media/pci/ttpci/av7110_av.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/ttpci/av7110_av.c b/drivers/media/pci/ttpci/av7110_av.c
index ef1bc17..1ff6062 100644
--- a/drivers/media/pci/ttpci/av7110_av.c
+++ b/drivers/media/pci/ttpci/av7110_av.c
@@ -468,6 +468,8 @@ static ssize_t ts_play(struct av7110 *av7110, const char __user *buf,
 		}
 		if (copy_from_user(kb, buf, TS_SIZE))
 			return -EFAULT;
+		if (kb[0] != 0x47)
+			return -EINVAL;
 		write_ts_to_decoder(av7110, type, kb, TS_SIZE);
 		todo -= TS_SIZE;
 		buf += TS_SIZE;
-- 
2.7.4