Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752019AbbFZXFL (ORCPT ); Fri, 26 Jun 2015 19:05:11 -0400 Received: from mail-ig0-f173.google.com ([209.85.213.173]:37935 "EHLO mail-ig0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751893AbbFZXFF (ORCPT ); Fri, 26 Jun 2015 19:05:05 -0400 Date: Fri, 26 Jun 2015 16:05:00 -0700 From: Dmitry Torokhov To: cpaul@redhat.com Cc: Benjamin Tissoires , linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, Hans de Goede Subject: Re: [PATCH v3] i8042: Add debug_kbd option Message-ID: <20150626230500.GA22932@dtor-ws> References: <1435260310-10074-1-git-send-email-cpaul@redhat.com> <1435357698-13804-1-git-send-email-cpaul@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1435357698-13804-1-git-send-email-cpaul@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8255 Lines: 241 Hi Stephen, On Fri, Jun 26, 2015 at 06:28:18PM -0400, cpaul@redhat.com wrote: > From: Stephen Chandler Paul > > A big problem with the current i8042 debugging option is that it outputs > data going to and from the keyboard by default. As a result, many dmesg > logs uploaded by users will unintentionally contain sensitive information > such as their password, as such it's probably a good idea not to output > data coming from the keyboard unless specifically enabled by the user. > > Signed-off-by: Stephen Chandler Paul > --- > That patch was not working at all like I thought it was, the results on the > machine I tested it on were very misleading so I apologize for any trouble that > may have caused! I've done more thorough testing of this, and it should work > perfectly on any machine. > > Changes > * Bus structs are shared between devices, whoops. Now when the notifier goes > off we check two things: > * That the device actually belongs to the i8042 platform driver. This is > something that's difficult to reproduce, since devices on the i8042 bus > are usually handled by the plaform driver. If they don't happen to be > however (which currently I've only seen with ps2emu, a kernel module I'm > working on to replay PS/2 devices in the kernel), then we make the false > assumption the port_data variable points to an i8042_port struct, and > very likely crash the machine or worse. > * That the IRQ of the port is equivalent to that of the KBD port, so that > we only mask the interrupts coming from the keyboard. > > Documentation/kernel-parameters.txt | 3 ++ > drivers/input/serio/i8042.c | 64 +++++++++++++++++++++++++++++++++---- > drivers/input/serio/i8042.h | 13 ++++++++ > 3 files changed, 74 insertions(+), 6 deletions(-) > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index ae44749..a9d2b19 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -1304,6 +1304,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted. > , > > i8042.debug [HW] Toggle i8042 debug mode > + i8042.debug_kbd [HW] Enable printing of interrupt data from the KBD port > + (disabled by default, requires that i8042.debug=1 > + be enabled) > i8042.direct [HW] Put keyboard port into non-translated mode > i8042.dumbkbd [HW] Pretend that controller can only read data from > keyboard and cannot control its state > diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c > index cb5ece7..0e17bdd 100644 > --- a/drivers/input/serio/i8042.c > +++ b/drivers/input/serio/i8042.c > @@ -88,6 +88,10 @@ MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings"); > static bool i8042_debug; > module_param_named(debug, i8042_debug, bool, 0600); > MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off"); > + > +static bool i8042_debug_kbd; > +module_param_named(debug_kbd, i8042_debug_kbd, bool, 0600); > +MODULE_PARM_DESC(i8042_kbd, "Turn i8042 kbd debugging output on or off (requires i8042.debug=1)"); > #endif > > static bool i8042_bypass_aux_irq_test; > @@ -116,6 +120,9 @@ struct i8042_port { > struct serio *serio; > int irq; > bool exists; > +#ifdef DEBUG > + bool filter_dbg; > +#endif Could we call it "driver_bound" and drop #ifdef DEBUG? In fact, I'd rager we dropped the #ifdef DEBUG arounf all bus notifier code. > signed char mux; > }; > > @@ -133,6 +140,9 @@ static bool i8042_kbd_irq_registered; > static bool i8042_aux_irq_registered; > static unsigned char i8042_suppress_kbd_ack; > static struct platform_device *i8042_platform_device; > +#ifdef DEBUG > +static struct notifier_block i8042_kbd_bind_notifier_block; > +#endif > > static irqreturn_t i8042_interrupt(int irq, void *dev_id); > static bool (*i8042_platform_filter)(unsigned char data, unsigned char str, > @@ -528,10 +538,10 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) > port = &i8042_ports[port_no]; > serio = port->exists ? port->serio : NULL; > > - dbg("%02x <- i8042 (interrupt, %d, %d%s%s)\n", > - data, port_no, irq, > - dfl & SERIO_PARITY ? ", bad parity" : "", > - dfl & SERIO_TIMEOUT ? ", timeout" : ""); > + filter_dbg(port->filter_dbg, data, "<- i8042 (interrupt, %d, %d%s%s)\n", > + port_no, irq, > + dfl & SERIO_PARITY ? ", bad parity" : "", > + dfl & SERIO_TIMEOUT ? ", timeout" : ""); > > filtered = i8042_filter(data, str, serio); > > @@ -1329,6 +1339,13 @@ static void __init i8042_register_ports(void) > i8042_ports[i].irq); > serio_register_port(serio); > device_set_wakeup_capable(&serio->dev, true); > +#ifdef DEBUG > + if (i == I8042_KBD_PORT_NO) { > + bus_register_notifier( > + serio->dev.bus, > + &i8042_kbd_bind_notifier_block); > + } Umm, let's export serio_bus and register the notifier when we load the module. > +#endif > } > } > } > @@ -1338,8 +1355,17 @@ static void i8042_unregister_ports(void) > int i; > > for (i = 0; i < I8042_NUM_PORTS; i++) { > - if (i8042_ports[i].serio) { > - serio_unregister_port(i8042_ports[i].serio); > + struct serio *serio = i8042_ports[i].serio; > + > + if (serio) { > +#ifdef DEBUG > + if (i == I8042_KBD_PORT_NO) { > + bus_unregister_notifier( > + serio->dev.bus, > + &i8042_kbd_bind_notifier_block); > + } > +#endif > + serio_unregister_port(serio); > i8042_ports[i].serio = NULL; > } > } > @@ -1438,6 +1464,26 @@ static int __init i8042_setup_kbd(void) > return error; > } > > +#ifdef DEBUG > +static int i8042_kbd_bind_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct device *dev = data; > + struct i8042_port *port = to_serio_port(dev)->port_data; > + > + if (dev->parent != &i8042_platform_device->dev || > + port->irq != I8042_KBD_IRQ) > + return 0; Why don't you compare serio with i8042_ports[I8042_KBD_PORT_NO].serio instead? > + > + if (action == BUS_NOTIFY_BOUND_DRIVER) > + port->filter_dbg = true; > + else if (action == BUS_NOTIFY_UNBOUND_DRIVER) > + port->filter_dbg = false; switch (action) { } please. > + > + return 0; > +} > +#endif > + > static int __init i8042_probe(struct platform_device *dev) > { > int error; > @@ -1507,6 +1553,12 @@ static struct platform_driver i8042_driver = { > .shutdown = i8042_shutdown, > }; > > +#ifdef DEBUG > +static struct notifier_block i8042_kbd_bind_notifier_block = { > + .notifier_call = i8042_kbd_bind_notifier, > +}; > +#endif > + > static int __init i8042_init(void) > { > struct platform_device *pdev; > diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h > index fc080be..a198f0d 100644 > --- a/drivers/input/serio/i8042.h > +++ b/drivers/input/serio/i8042.h > @@ -73,6 +73,17 @@ static unsigned long i8042_start_time; > printk(KERN_DEBUG KBUILD_MODNAME ": [%d] " format, \ > (int) (jiffies - i8042_start_time), ##arg); \ > } while (0) > + > +#define filter_dbg(filter, data, format, args...) \ > + do { \ > + if (!i8042_debug) \ > + break; \ > + \ > + if (!filter || i8042_debug_kbd) \ > + dbg("%02x " format, data, ##args); \ > + else \ > + dbg("** " format, ##args); \ > + } while (0) > #else > #define dbg_init() do { } while (0) > #define dbg(format, arg...) \ > @@ -80,6 +91,8 @@ static unsigned long i8042_start_time; > if (0) \ > printk(KERN_DEBUG pr_fmt(format), ##arg); \ > } while (0) > + > +#define filter_dbg(filter, data, format, args...) do { } while (0) > #endif > > #endif /* _I8042_H */ > -- > 2.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/