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

[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 <sean@mess.org>
---
 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(&dev->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(&dev->timer_keyup, dev->keyup_jiffies);
 	}
 	spin_unlock_irqrestore(&dev->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 infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] media: rc: repeat_period() can be static

[kbuild test robot]

Fixes: f52e85dbaa91 ("media: rc: read out of bounds if bpf reports high protocol number")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 rc-main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index a24850b..ca68e1d 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -679,7 +679,7 @@ static void ir_timer_repeat(struct timer_list *t)
 	spin_unlock_irqrestore(&dev->keylock, flags);
 }
 
-unsigned int repeat_period(int protocol)
+static unsigned int repeat_period(int protocol)
 {
 	if (protocol >= ARRAY_SIZE(protocols))
 		return 100;

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 <sean@mess.org>
> ---
>  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(&dev->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(&dev->timer_keyup, dev->keyup_jiffies);
>  	}
>  	spin_unlock_irqrestore(&dev->keylock, flags);
> -- 
> 2.17.1
> 

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 <sean@mess.org>
> > ---
> >  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(&dev->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