Re: [PATCH v3] i8042: Add debug_kbd option
From: Dmitry Torokhov
Date: Fri Jun 26 2015 - 19:05:14 EST
Hi Stephen,
On Fri, Jun 26, 2015 at 06:28:18PM -0400, cpaul@xxxxxxxxxx wrote:
> From: Stephen Chandler Paul <cpaul@xxxxxxxxxx>
>
> 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 <cpaul@xxxxxxxxxx>
> ---
> 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.
> <bus_id>,<clkrate>
>
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/