Message ID | 20180728091115.16971-1-sean@mess.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: rc: read out of bounds if bpf reports high protocol number | expand |
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
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 >
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
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);
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(-)