linux-kernel - Re: [PATCH 1/4] Input: make sure input handlers define only one processing method

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZoLYscmmUR7Fu76F@google.com>
Date: Mon, 1 Jul 2024 09:26:25 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: linux-input@...r.kernel.org, Jeff LaBundy <jeff@...undy.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] Input: make sure input handlers define only one
 processing method

Hi Benjamin,

On Mon, Jul 01, 2024 at 09:36:08AM +0200, Benjamin Tissoires wrote:
> Hi Dmitry,
> 
> On Jun 30 2024, Dmitry Torokhov wrote:
> > Input core expects input handlers to be either filters, or regular
> > handlers, but not both. Additionally, for regular handlers it does
> > not make sense to define both single event method and batch method.
> > 
> > Refuse registering handler if it defines more than one method.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > ---
> >  drivers/input/input.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index fd4997ba263c..8434348faeac 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -2517,6 +2517,26 @@ void input_unregister_device(struct input_dev *dev)
> >  }
> >  EXPORT_SYMBOL(input_unregister_device);
> >  
> > +static int input_handler_check_methods(const struct input_handler *handler)
> > +{
> > +	int count = 0;
> > +
> > +	if (handler->filter)
> > +		count++;
> > +	if (handler->events)
> > +		count++;
> > +	if (handler->event)
> > +		count++;
> > +
> > +	if (count != 1) {
> 
> Am I missing some upstream commit? I have the following:

Thank you for the thorough review!

> 
> in drivers/input/evdev.c:
> 
> static struct input_handler evdev_handler = {
> 	.event		= evdev_event,
> 	.events		= evdev_events,
> 	.connect	= evdev_connect,
> 	.disconnect	= evdev_disconnect,
> 	.legacy_minors	= true,
> 	.minor		= EVDEV_MINOR_BASE,
> 	.name		= "evdev",
> 	.id_table	= evdev_ids,
> };
> 
> So here count should be 2 and evdev would be rejected?

Uh, it looks like I had a patch buried that removed evdev_event as not
needed - if a handler has ->events() then input core would favor it even
before my patches.

> 
> And in drivers/tty/serial/kgdboc.c:
> 
> static struct input_handler kgdboc_reset_handler = {
> 	.connect	= kgdboc_reset_connect,
> 	.disconnect	= kgdboc_reset_disconnect,
> 	.name		= "kgdboc_reset",
> 	.id_table	= kgdboc_reset_ids,
> };
> 
> here count would be 0 and kgdboc would also be rejected.

Yes, you are totally right. It looks like we need to allow the "no methods"
case.

> 
> I agree on the intent of the patch, but these couple of input handlers
> should be fixed if they are not already.

Yep, I will address your other comments and resend in a few.

Thanks again!

-- 
Dmitry