Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number

[Sean Young]

Hi Hias,

On Mon, Jul 30, 2018 at 09:20:18PM +0200, Matthias Reichl wrote:
> On Sat, Jul 28, 2018 at 10:11:15AM +0100, Sean Young wrote:
> > The repeat period is read from a static array. If a keydown event is
> > reported from bpf with a high protocol number, we read out of bounds. This
> > is unlikely to end up with a reasonable repeat period at the best of times,
> > in which case no timely key up event is generated.
> > 
> > Signed-off-by: Sean Young 
> > ---
> >  drivers/media/rc/rc-main.c | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> > index 2e222d9ee01f..a24850be1f4f 100644
> > --- a/drivers/media/rc/rc-main.c
> > +++ b/drivers/media/rc/rc-main.c
> > @@ -679,6 +679,14 @@ static void ir_timer_repeat(struct timer_list *t)
> > spin_unlock_irqrestore(>keylock, flags);
> >  }
> >  
> > +unsigned int repeat_period(int protocol)
> > +{
> > +   if (protocol >= ARRAY_SIZE(protocols))
> > +   return 100;
> 
> 100 seems a bit arbitrarily chosen to me. Wouldn't it be better to
> (re-)use eg protocols[RC_PROTO_UNKNOWN].repeat_period here?

That's a good idea! I think the patch is already on its way to be merged,
but we can patch this later.

What we really need is a way to set the repeat period and minimum timeout
for a bpf protocol.


Sean

Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number

[Matthias Reichl]

Hi Sean,

On Sat, Jul 28, 2018 at 10:11:15AM +0100, Sean Young wrote:
> The repeat period is read from a static array. If a keydown event is
> reported from bpf with a high protocol number, we read out of bounds. This
> is unlikely to end up with a reasonable repeat period at the best of times,
> in which case no timely key up event is generated.
> 
> Signed-off-by: Sean Young 
> ---
>  drivers/media/rc/rc-main.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 2e222d9ee01f..a24850be1f4f 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -679,6 +679,14 @@ static void ir_timer_repeat(struct timer_list *t)
>   spin_unlock_irqrestore(>keylock, flags);
>  }
>  
> +unsigned int repeat_period(int protocol)
> +{
> + if (protocol >= ARRAY_SIZE(protocols))
> + return 100;

100 seems a bit arbitrarily chosen to me. Wouldn't it be better to
(re-)use eg protocols[RC_PROTO_UNKNOWN].repeat_period here?

so long,

Hias

> +
> + return protocols[protocol].repeat_period;
> +}
> +
>  /**
>   * rc_repeat() - signals that a key is still pressed
>   * @dev: the struct rc_dev descriptor of the device
> @@ -691,7 +699,7 @@ void rc_repeat(struct rc_dev *dev)
>  {
>   unsigned long flags;
>   unsigned int timeout = nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[dev->last_protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(dev->last_protocol));
>   struct lirc_scancode sc = {
>   .scancode = dev->last_scancode, .rc_proto = dev->last_protocol,
>   .keycode = dev->keypressed ? dev->last_keycode : KEY_RESERVED,
> @@ -803,7 +811,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto 
> protocol, u32 scancode,
>  
>   if (dev->keypressed) {
>   dev->keyup_jiffies = jiffies + nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(protocol));
>   mod_timer(>timer_keyup, dev->keyup_jiffies);
>   }
>   spin_unlock_irqrestore(>keylock, flags);
> -- 
> 2.17.1
> 

Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number

[kbuild test robot]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Sean-Young/media-rc-read-out-of-bounds-if-bpf-reports-high-protocol-number/20180729-035942
base:   git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/media/rc/rc-main.c:682:14: sparse: symbol 'repeat_period' was not 
>> declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

[PATCH] media: rc: read out of bounds if bpf reports high protocol number

[Sean Young]

The repeat period is read from a static array. If a keydown event is
reported from bpf with a high protocol number, we read out of bounds. This
is unlikely to end up with a reasonable repeat period at the best of times,
in which case no timely key up event is generated.

Signed-off-by: Sean Young 
---
 drivers/media/rc/rc-main.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 2e222d9ee01f..a24850be1f4f 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -679,6 +679,14 @@ static void ir_timer_repeat(struct timer_list *t)
spin_unlock_irqrestore(>keylock, flags);
 }
 
+unsigned int repeat_period(int protocol)
+{
+   if (protocol >= ARRAY_SIZE(protocols))
+   return 100;
+
+   return protocols[protocol].repeat_period;
+}
+
 /**
  * rc_repeat() - signals that a key is still pressed
  * @dev:   the struct rc_dev descriptor of the device
@@ -691,7 +699,7 @@ void rc_repeat(struct rc_dev *dev)
 {
unsigned long flags;
unsigned int timeout = nsecs_to_jiffies(dev->timeout) +
-   msecs_to_jiffies(protocols[dev->last_protocol].repeat_period);
+   msecs_to_jiffies(repeat_period(dev->last_protocol));
struct lirc_scancode sc = {
.scancode = dev->last_scancode, .rc_proto = dev->last_protocol,
.keycode = dev->keypressed ? dev->last_keycode : KEY_RESERVED,
@@ -803,7 +811,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, 
u32 scancode,
 
if (dev->keypressed) {
dev->keyup_jiffies = jiffies + nsecs_to_jiffies(dev->timeout) +
-   msecs_to_jiffies(protocols[protocol].repeat_period);
+   msecs_to_jiffies(repeat_period(protocol));
mod_timer(>timer_keyup, dev->keyup_jiffies);
}
spin_unlock_irqrestore(>keylock, flags);
-- 
2.17.1