Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number
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(&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
Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number
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(&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
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
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(&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