Re: [PATCH v3] i8042: Add debug_kbd option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux