Re: [PATCH v3] i8042: Add debug_kbd option
From: Dmitry Torokhov
Date: Fri Jun 26 2015 - 19:24:59 EST
On Fri, Jun 26, 2015 at 07:21:47PM -0400, Stephen Chandler Paul wrote:
> On Fri, 2015-06-26 at 16:05 -0700, Dmitry Torokhov wrote:
> > 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_b
> > > lock);
> > > + }
> >
> >
> > Umm, let's export serio_bus and register the notifier when we load
> > the
> > module.
> >
>
> Sorry, but would you mind clarifying what you mean by "export
> serio_bus"?
Right now in serio.c we have:
static struct bus_type serio_bus;
and that is why you have to go by serio->dev.bus to access it. If you
export the symbol you can use it directly, before you allocate any
serio ports.
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/