* Re: [PATCH] Input: atmel_mxt_ts - update to devm_* API
From: Dmitry Torokhov @ 2013-09-16 15:06 UTC (permalink / raw)
To: Manish Badarkhe; +Cc: linux-input, linux-kernel, josh.wu
In-Reply-To: <1379343016-6354-1-git-send-email-badarkhe.manish@gmail.com>
Hi Manish,
On Mon, Sep 16, 2013 at 08:20:16PM +0530, Manish Badarkhe wrote:
> @@ -1264,10 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
> struct mxt_data *data = i2c_get_clientdata(client);
>
> sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> - free_irq(data->irq, data);
> input_unregister_device(data->input_dev);
> kfree(data->object_table);
> - kfree(data);
YOu need to be _very_ careful when mixing devm_* and non-devm-managed
resources, especially when interrupt is involved. In this particular
case you are offloading freeing of the interrupt (and disabling it) to
devm, which will happen later, bu you are freeing object table
immediately. This may cause driver crash if interrupt comes while device
is being unbound.
That said, there is a large outstanding series to the driver from Nick
Dryer that I am trying to merge, you may want to coordinate this change
with him.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Marek Vasut @ 2013-09-16 15:10 UTC (permalink / raw)
To: Jürgen Beisert
Cc: linux-arm-kernel, Jonathan Cameron, linux-iio, devel,
fabio.estevam, jic23, Torokhov, linux-input@vger.kernel.org,
Otavio Salvador
In-Reply-To: <201309161634.54603.jbe@pengutronix.de>
Dear Jürgen Beisert,
> Hi Marek,
>
> On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > > For battery driven systems it is a very bad idea to collect the
> > > > > touchscreen data within a kernel busy loop.
> > > > >
> > > > > This change uses the features of the hardware to delay and
> > > > > accumulate samples in hardware to avoid a high interrupt and CPU
> > > > > load.
> > > > >
> > > > > Note: this is only tested on an i.MX23 SoC yet.
> > > > >
> > > > > Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> > > > > CC: linux-arm-kernel@lists.infradead.org
> > > > > CC: devel@driverdev.osuosl.org
> > > > > CC: Marek Vasut <marex@denx.de>
> > > > > CC: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > CC: Jonathan Cameron <jic23@cam.ac.uk>
> > > >
> > > > While this driver is placed in IIO within staging at the moment,
> > > > these changes are definitely input related. Hence I have cc'd
> > > > Dmitry and the input list.
> > > >
> > > > I am personaly a little uncomfortable that we have such a complex bit
> > > > of input code sat within an IIO driver but such is life.
> > >
> > > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > > have a different problem with this driver, because the ADC unit
> > > monitors the battery as well. And the charging driver from the power
> > > subsystem needs these values to charge the battery in a correct
> > > manner.
> >
> > Are you planning to post the power block patches too ?
>
> When they will work: yes. Currently it crashes all the time. The PMIC is a
> beast...
Full ACK
Best regards,
Marek Vasut
--
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
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 15:20 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, linux-kernel, vojtech, linux-input, apw,
devel
In-Reply-To: <1379309334-25042-1-git-send-email-kys@microsoft.com>
Hi K. Y.
On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
>
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver.
>
In addition to what Dan said:
> +
> +struct synth_kbd_protocol_response {
> + struct synth_kbd_msg_hdr header;
> + u32 accepted:1;
> + u32 reserved:31;
> +};
Use of bitfields for on the wire structures makes me uneasy. I know that
currently you only going to run LE on LE, but still, maybe using
explicit shifts and masks would be better,
> +
> +struct synth_kbd_keystroke {
> + struct synth_kbd_msg_hdr header;
> + u16 make_code;
> + u16 reserved0;
> + u32 is_unicode:1;
> + u32 is_break:1;
> + u32 is_e0:1;
> + u32 is_e1:1;
> + u32 reserved:28;
> +};
Same here.
> +
> +
> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0 0xe0
> +#define XTKBD_EMUL1 0xe1
> +#define XTKBD_RELEASE 0x80
> +
> +
> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> + unsigned char keycode[256];
I do not see anything using keycode table? This is wrong level for it
regardless.
> + struct hv_device *device;
> + struct synth_kbd_protocol_request protocol_req;
> + struct synth_kbd_protocol_response protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + struct serio *hv_serio;
> +};
> +
> +
> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> + struct hv_kbd_dev *kbd_dev;
> + struct serio *hv_serio;
You are aligning with tabs half of declarations, leaving the others. Can
we not align at all?
> +
> + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +
> + if (!kbd_dev)
> + return NULL;
> +
> + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +
> + if (hv_serio == NULL) {
> + kfree(kbd_dev);
> + return NULL;
> + }
> +
> + hv_serio->id.type = SERIO_8042_XL;
> + strlcpy(hv_serio->name, dev_name(&device->device),
> + sizeof(hv_serio->name));
> + strlcpy(hv_serio->phys, dev_name(&device->device),
> + sizeof(hv_serio->phys));
> + hv_serio->dev.parent = &device->device;
Do you also want to set up serio's parent device to point to hv device?
> +
> +
> + kbd_dev->device = device;
> + kbd_dev->hv_serio = hv_serio;
> + hv_set_drvdata(device, kbd_dev);
> + init_completion(&kbd_dev->wait_event);
> +
> + return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> + serio_unregister_port(device->hv_serio);
> + kfree(device->hv_serio);
Serio ports are refcounted, do not free them explicitly after they have
been registered.
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Dmitry Torokhov @ 2013-09-16 15:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Juergen Beisert, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <52359259.8010202-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, Sep 15, 2013 at 11:56:25AM +0100, Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
> > For battery driven systems it is a very bad idea to collect the touchscreen
> > data within a kernel busy loop.
> >
> > This change uses the features of the hardware to delay and accumulate samples in
> > hardware to avoid a high interrupt and CPU load.
> >
> > Note: this is only tested on an i.MX23 SoC yet.
> >
> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> While this driver is placed in IIO within staging at the moment, these changes are definitely
> input related. Hence I have cc'd Dmitry and the input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of input code sat
> within an IIO driver but such is life.
>
...
> >
> > static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> > @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
> >
> > cancel_work_sync(&lradc->ts_work);
> >
> > + mxs_lradc_disable_ts(lradc);
> > input_unregister_device(lradc->ts_input);
> > }
This looks iffy... Normally you disable the device so that it does not
generate more interrupts, and then cancel outstanding work(s), otherwise
newly generated interrupts may cause more work to be scheduled. Or I
missed some of the context and this is not a concern here?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jonathan Cameron @ 2013-09-16 15:30 UTC (permalink / raw)
To: Jürgen Beisert,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <201309161010.22151.jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
"Jürgen Beisert" <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>Hi Jonathan,
>
>On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
>> On 09/11/13 09:18, Juergen Beisert wrote:
>> > For battery driven systems it is a very bad idea to collect the
>> > touchscreen data within a kernel busy loop.
>> >
>> > This change uses the features of the hardware to delay and
>accumulate
>> > samples in hardware to avoid a high interrupt and CPU load.
>> >
>> > Note: this is only tested on an i.MX23 SoC yet.
>> >
>> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>>
>> While this driver is placed in IIO within staging at the moment,
>these
>> changes are definitely input related. Hence I have cc'd Dmitry and
>the
>> input list.
>>
>> I am personaly a little uncomfortable that we have such a complex bit
>of
>> input code sat within an IIO driver but such is life.
>
>Maybe an MFD for this ADC unit would be a better way to go?
That would be great and is definitely the preferred method.
Currently I
>have a
>different problem with this driver, because the ADC unit monitors the
>battery
>as well. And the charging driver from the power subsystem needs these
>values
>to charge the battery in a correct manner.
There is an iio client battery driver as generic_ADC_battery.c but it is currently only doing polled access. Not too hard to add buffered access though ideally we would want a generic way of combining consumers doing polled and interrupt driven accesses.
>
>> [...]
>
>Regards,
>Juergen
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 15:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, linux-input@vger.kernel.org,
vojtech@suse.cz, olaf@aepfle.de, apw@canonical.com,
jasowang@redhat.com
In-Reply-To: <20130916152019.GB12105@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 8:20 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> Hi K. Y.
>
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
>
> In addition to what Dan said:
>
> > +
> > +struct synth_kbd_protocol_response {
> > + struct synth_kbd_msg_hdr header;
> > + u32 accepted:1;
> > + u32 reserved:31;
> > +};
>
> Use of bitfields for on the wire structures makes me uneasy. I know that
> currently you only going to run LE on LE, but still, maybe using
> explicit shifts and masks would be better,
This definition of the data structure is defined by the host. I will see what I
can do here.
>
> > +
> > +struct synth_kbd_keystroke {
> > + struct synth_kbd_msg_hdr header;
> > + u16 make_code;
> > + u16 reserved0;
> > + u32 is_unicode:1;
> > + u32 is_break:1;
> > + u32 is_e0:1;
> > + u32 is_e1:1;
> > + u32 reserved:28;
> > +};
>
> Same here.
>
> > +
> > +
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0 0xe0
> > +#define XTKBD_EMUL1 0xe1
> > +#define XTKBD_RELEASE 0x80
> > +
> > +
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > + unsigned char keycode[256];
>
> I do not see anything using keycode table? This is wrong level for it
> regardless.
>
> > + struct hv_device *device;
> > + struct synth_kbd_protocol_request protocol_req;
> > + struct synth_kbd_protocol_response protocol_resp;
> > + /* Synchronize the request/response if needed */
> > + struct completion wait_event;
> > + struct serio *hv_serio;
> > +};
> > +
> > +
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > + struct hv_kbd_dev *kbd_dev;
> > + struct serio *hv_serio;
>
> You are aligning with tabs half of declarations, leaving the others. Can
> we not align at all?
>
> > +
> > + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
> > + if (!kbd_dev)
> > + return NULL;
> > +
> > + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
> > + if (hv_serio == NULL) {
> > + kfree(kbd_dev);
> > + return NULL;
> > + }
> > +
> > + hv_serio->id.type = SERIO_8042_XL;
> > + strlcpy(hv_serio->name, dev_name(&device->device),
> > + sizeof(hv_serio->name));
> > + strlcpy(hv_serio->phys, dev_name(&device->device),
> > + sizeof(hv_serio->phys));
> > + hv_serio->dev.parent = &device->device;
>
> Do you also want to set up serio's parent device to point to hv device?
Is there an issue here; I could setup the parent as the vmbus device.
>
> > +
> > +
> > + kbd_dev->device = device;
> > + kbd_dev->hv_serio = hv_serio;
> > + hv_set_drvdata(device, kbd_dev);
> > + init_completion(&kbd_dev->wait_event);
> > +
> > + return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > + serio_unregister_port(device->hv_serio);
> > + kfree(device->hv_serio);
>
> Serio ports are refcounted, do not free them explicitly after they have
> been registered.
Thank you. I will fix this.
>
> > + hv_set_drvdata(device->device, NULL);
> > + kfree(device);
> > +}
>
>
> Thanks.
Thank you!.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Marek Vasut @ 2013-09-16 16:49 UTC (permalink / raw)
To: Mike Dunn; +Cc: linux-input, Dmitry Torokhov, Chao Xie, Robert Jarzmik
In-Reply-To: <1379349842-13089-1-git-send-email-mikedunn@newsguy.com>
Dear Mike Dunn,
> A NULL pointer dereference exception occurs in the driver probe function
> when device tree is used. The pdata pointer will be NULL in this case,
> but the code dereferences it in all cases. When device tree is used, a
> platform data structure is allocated and initialized, and in all cases
> this pointer is copied to the driver's private data, so the variable being
> tested should be accessed through the driver's private data structure.
>
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
> --- a/drivers/input/keyboard/pxa27x_keypad.c
> +++ b/drivers/input/keyboard/pxa27x_keypad.c
> @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
> *pdev) goto failed_put_clk;
> }
>
> - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
> - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
> + if ((keypad->pdata->enable_rotary0 &&
> + keypad->rotary_rel_code[0] != -1) ||
> + (keypad->pdata->enable_rotary1 &&
> + keypad->rotary_rel_code[1] != -1)) {
> input_dev->evbit[0] |= BIT_MASK(EV_REL);
> }
Nice find. Acked-by: Marek Vasut <marex@denx.de>
Best regards,
Marek Vasut
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 16:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916150548.GO25896@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 8:06 AM
> To: KY Srinivasan
> Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org; vojtech@suse.cz;
> linux-input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > + case VM_PKT_DATA_INBAND:
> > > > + hv_kbd_on_receive(device, desc);
> > >
> > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > doesn't take into consideration the amount of data we recieved, it
> > > trusts the offset we recieved from the user. There is an out of bounds
> > > read.
> >
> > What user are you referring to. The message is sent by the host - the user
> keystroke
> > is normalized into a fixed size packet by the host and sent to the guest. We will
> parse this
> > packet, based on the host specified layout here.
> >
>
> The user means the hypervisor, yes.
>
> I don't want the hypervisor accessing outside of the buffer. It is
> robustness issue. Just check the offset against "bytes_recvd". It's
> not complicated.
At the outset, let me apologize for not understanding your concern.
You say: " I don't want the hypervisor accessing outside of the buffer"
Where did you see the hypervisor accessing anything outside the buffer?
The buffer is allocated by this driver and a packet from vmbus is read into this
buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
than the packet that needs to be read, then nothing will be read. Once the read
completes, we can be sure we have read a valid packet and can proceed to parse it in
this driver.
I don't want to be argumentative, I am just not seeing the issue.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Dmitry Torokhov @ 2013-09-16 17:06 UTC (permalink / raw)
To: Marek Vasut; +Cc: Mike Dunn, linux-input, Chao Xie, Robert Jarzmik
In-Reply-To: <201309161849.53556.marex@denx.de>
On Mon, Sep 16, 2013 at 06:49:53PM +0200, Marek Vasut wrote:
> Dear Mike Dunn,
>
> > A NULL pointer dereference exception occurs in the driver probe function
> > when device tree is used. The pdata pointer will be NULL in this case,
> > but the code dereferences it in all cases. When device tree is used, a
> > platform data structure is allocated and initialized, and in all cases
> > this pointer is copied to the driver's private data, so the variable being
> > tested should be accessed through the driver's private data structure.
> >
> > Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> > ---
> > drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> > b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
> > --- a/drivers/input/keyboard/pxa27x_keypad.c
> > +++ b/drivers/input/keyboard/pxa27x_keypad.c
> > @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
> > *pdev) goto failed_put_clk;
> > }
> >
> > - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
> > - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
> > + if ((keypad->pdata->enable_rotary0 &&
> > + keypad->rotary_rel_code[0] != -1) ||
> > + (keypad->pdata->enable_rotary1 &&
> > + keypad->rotary_rel_code[1] != -1)) {
> > input_dev->evbit[0] |= BIT_MASK(EV_REL);
> > }
>
> Nice find. Acked-by: Marek Vasut <marex@denx.de>
Excellent booby trap. I would prefer if we explicitly did
pdata = keypad->pdata;
after calling the parse DT fucntion with a nice comment, because we
somebody might want to rearrange the code and accidentially revert the
checks to the original state.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Mike Dunn @ 2013-09-16 16:44 UTC (permalink / raw)
To: linux-input
Cc: Mike Dunn, Dmitry Torokhov, Chao Xie, Robert Jarzmik, Marek Vasut
A NULL pointer dereference exception occurs in the driver probe function when
device tree is used. The pdata pointer will be NULL in this case, but the code
dereferences it in all cases. When device tree is used, a platform data
structure is allocated and initialized, and in all cases this pointer is copied
to the driver's private data, so the variable being tested should be accessed
through the driver's private data structure.
Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c
index 134c3b4..3b2a614 100644
--- a/drivers/input/keyboard/pxa27x_keypad.c
+++ b/drivers/input/keyboard/pxa27x_keypad.c
@@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device *pdev)
goto failed_put_clk;
}
- if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
- (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
+ if ((keypad->pdata->enable_rotary0 &&
+ keypad->rotary_rel_code[0] != -1) ||
+ (keypad->pdata->enable_rotary1 &&
+ keypad->rotary_rel_code[1] != -1)) {
input_dev->evbit[0] |= BIT_MASK(EV_REL);
}
--
1.8.1.5
^ permalink raw reply related
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 17:09 UTC (permalink / raw)
To: KY Srinivasan
Cc: Dan Carpenter, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <baabf219b12b40de8e17b029e621e079@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, September 16, 2013 8:06 AM
> > To: KY Srinivasan
> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org; vojtech@suse.cz;
> > linux-input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > + case VM_PKT_DATA_INBAND:
> > > > > + hv_kbd_on_receive(device, desc);
> > > >
> > > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > > doesn't take into consideration the amount of data we recieved, it
> > > > trusts the offset we recieved from the user. There is an out of bounds
> > > > read.
> > >
> > > What user are you referring to. The message is sent by the host - the user
> > keystroke
> > > is normalized into a fixed size packet by the host and sent to the guest. We will
> > parse this
> > > packet, based on the host specified layout here.
> > >
> >
> > The user means the hypervisor, yes.
> >
> > I don't want the hypervisor accessing outside of the buffer. It is
> > robustness issue. Just check the offset against "bytes_recvd". It's
> > not complicated.
>
> At the outset, let me apologize for not understanding your concern.
> You say: " I don't want the hypervisor accessing outside of the buffer"
> Where did you see the hypervisor accessing anything outside the buffer?
> The buffer is allocated by this driver and a packet from vmbus is read into this
> buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> than the packet that needs to be read, then nothing will be read. Once the read
> completes, we can be sure we have read a valid packet and can proceed to parse it in
> this driver.
The concern is that number of bytes received and contents of a packet
are not in sync. Imagine if we were told that 16 butes was received but
in the packet offset is 78. Then we'll try reading well past the buffer
boundary that we allocated for the packets.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 17:13 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <076938490f5141ef957947dc10b82c65@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 03:52:18PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Monday, September 16, 2013 8:20 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > Hi K. Y.
> >
> > On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > > Add a new driver to support synthetic keyboard. On the next generation
> > > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > > driver will be required.
> > >
> > > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > > details of the AT keyboard driver.
> > >
> >
> > In addition to what Dan said:
> >
> > > +
> > > +struct synth_kbd_protocol_response {
> > > + struct synth_kbd_msg_hdr header;
> > > + u32 accepted:1;
> > > + u32 reserved:31;
> > > +};
> >
> > Use of bitfields for on the wire structures makes me uneasy. I know that
> > currently you only going to run LE on LE, but still, maybe using
> > explicit shifts and masks would be better,
>
> This definition of the data structure is defined by the host. I will see what I
> can do here.
You do not really need to change protocol, you just sat that accepted is
the bit 0 of the word and define endianness (LE in your case). Then you
do:
struct synth_kbd_protocol_response {
struct synth_kbd_msg_hdr header;
__le32 status;
}
#define KBD_PROTOCOL_ACCEPTED BIT(0)
...
status = _le32_to_cpu(response->status);
accepted = status & KBD_PROTOCOL_ACCEPTED;
Thanks.
--
Dmitry
^ permalink raw reply
* [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Joseph Salisbury @ 2013-09-16 17:42 UTC (permalink / raw)
To: thomas, Jiri Kosina
Cc: list, Haiyang Zhang, LKML, open, HID CORE LAYER, devel
Hi Thomas,
A bug was opened against the Ubuntu kernel[0]. After a kernel bisect,
it was found that reverting the following commit resolved
this bug:
commit a4a23f6d68ad2c86ee8df6a6f89c9d315c0a761c
Author: Thomas Meyer <thomas@m3y3r.de>
Date: Sat Jun 1 11:40:31 2013 +0200
HID: hyperv: convert alloc+memcpy to memdup
The regression was introduced as of v3.11-rc1. The bug causes a system
to lockup when connecting a bluetooth trackpad and has been reported on
multiple machines.
I wanted to ping you since you are the author of this patch. I was
thinking of requesting a revert for v3.12 and v3.11, but I wanted to get
your feedback first.
Reverting the patch changes the driver back to useing kzalloc() and
memcpy() instead of kmemdup. Doing so has uncovered another bug, which
causes an oops on memcpy()[1]. We are in the process of bisecting that
one now and will provide the results.
Thanks,
Joe
[0] pad.lv/1218004
[1]
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1218004/+attachment/3818671/+files/kern.log
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 18:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dan Carpenter, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916170951.GB20734@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 10:10 AM
> To: KY Srinivasan
> Cc: Dan Carpenter; olaf@aepfle.de; gregkh@linuxfoundation.org;
> jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Monday, September 16, 2013 8:06 AM
> > > To: KY Srinivasan
> > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > > dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org;
> vojtech@suse.cz;
> > > linux-input@vger.kernel.org; apw@canonical.com;
> devel@linuxdriverproject.org
> > > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > > synthetic keyboard
> > >
> > > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > > + case VM_PKT_DATA_INBAND:
> > > > > > + hv_kbd_on_receive(device, desc);
> > > > >
> > > > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > > > doesn't take into consideration the amount of data we recieved, it
> > > > > trusts the offset we recieved from the user. There is an out of bounds
> > > > > read.
> > > >
> > > > What user are you referring to. The message is sent by the host - the user
> > > keystroke
> > > > is normalized into a fixed size packet by the host and sent to the guest. We
> will
> > > parse this
> > > > packet, based on the host specified layout here.
> > > >
> > >
> > > The user means the hypervisor, yes.
> > >
> > > I don't want the hypervisor accessing outside of the buffer. It is
> > > robustness issue. Just check the offset against "bytes_recvd". It's
> > > not complicated.
> >
> > At the outset, let me apologize for not understanding your concern.
> > You say: " I don't want the hypervisor accessing outside of the buffer"
> > Where did you see the hypervisor accessing anything outside the buffer?
> > The buffer is allocated by this driver and a packet from vmbus is read into this
> > buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> > than the packet that needs to be read, then nothing will be read. Once the read
> > completes, we can be sure we have read a valid packet and can proceed to
> parse it in
> > this driver.
>
> The concern is that number of bytes received and contents of a packet
> are not in sync. Imagine if we were told that 16 butes was received but
> in the packet offset is 78. Then we'll try reading well past the buffer
> boundary that we allocated for the packets.
I am not sure how this would be the case. Following are the semantics of the function
vmbus_recvpacket_raw():
If the packet to be read is larger than the buffer specified; nothing will be read and
appropriate error is returned. If a packet is read, the complete packet is read and
so we can safely peek into this packet based on the information in the header.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 18:33 UTC (permalink / raw)
To: KY Srinivasan
Cc: Dmitry Torokhov, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <3e45f054c6e243eabf2896ab1dcf38d4@SN2PR03MB061.namprd03.prod.outlook.com>
Just roll something like the following into your patch.
regards,
dan carpenter
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 0d4625f..262721b 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev *device)
}
static void hv_kbd_on_receive(struct hv_device *device,
- struct vmpacket_descriptor *packet)
+ struct vmpacket_descriptor *packet, size_t size)
{
struct synth_kbd_msg *msg;
struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
struct synth_kbd_keystroke *ks_msg;
+ int offset;
u16 scan_code;
- msg = (struct synth_kbd_msg *)((unsigned long)packet +
- (packet->offset8 << 3));
+ offset = packet->offset8 << 3;
+ if (offset + sizeof(struct synth_kbd_protocol_response) > size)
+ return;
+ msg = (void *)packet + offset;
switch (msg->header.type) {
case SYNTH_KBD_PROTOCOL_RESPONSE:
@@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context)
break;
case VM_PKT_DATA_INBAND:
- hv_kbd_on_receive(device, desc);
+ hv_kbd_on_receive(device, desc, bytes_recvd);
break;
default:
^ permalink raw reply related
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 18:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916183319.GM19256@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 11:33 AM
> To: KY Srinivasan
> Cc: Dmitry Torokhov; olaf@aepfle.de; gregkh@linuxfoundation.org;
> jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> Just roll something like the following into your patch.
>
> regards,
> dan carpenter
>
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-
> keyboard.c
> index 0d4625f..262721b 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev
> *device)
> }
>
> static void hv_kbd_on_receive(struct hv_device *device,
> - struct vmpacket_descriptor *packet)
> + struct vmpacket_descriptor *packet, size_t size)
> {
> struct synth_kbd_msg *msg;
> struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> struct synth_kbd_keystroke *ks_msg;
> + int offset;
> u16 scan_code;
>
> - msg = (struct synth_kbd_msg *)((unsigned long)packet +
> - (packet->offset8 << 3));
> + offset = packet->offset8 << 3;
> + if (offset + sizeof(struct synth_kbd_protocol_response) > size)
> + return;
> + msg = (void *)packet + offset;
>
> switch (msg->header.type) {
> case SYNTH_KBD_PROTOCOL_RESPONSE:
> @@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context)
> break;
>
> case VM_PKT_DATA_INBAND:
> - hv_kbd_on_receive(device, desc);
> + hv_kbd_on_receive(device, desc, bytes_recvd);
> break;
>
> default:
Dan,
Rolling the changes you have indicated is not the issue; this can trivially be done.
My contention is that it is not needed given that the underlying function is already
doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 20:13 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <117fbf407a554684b4d72ab06b033eeb@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote:
> Dan,
>
> Rolling the changes you have indicated is not the issue; this can trivially be done.
> My contention is that it is not needed given that the underlying function is already
> doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c.
>
I'm confused.
There is no mention of ->offset8 in vmbus_recvpacket_raw().
It's a good idea to add a check there but the lower levels don't know
about the sizeof(synth_kbd_protocol_response) so we would still need
something like my check.
regards,
dan carpenter
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Dan Carpenter @ 2013-09-16 20:38 UTC (permalink / raw)
To: Joseph Salisbury
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <5237430B.5040009@canonical.com>
On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> Reverting the patch changes the driver back to useing kzalloc() and
> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> causes an oops on memcpy()[1]. We are in the process of bisecting that
> one now and will provide the results.
The two bugs are the same it's that the code has shifted a little. Mark
the commit as buggy and continue with the git bisect.
regards,
dan carpenter
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Joseph Salisbury @ 2013-09-16 20:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <20130916203824.GP25896@mwanda>
On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>> Reverting the patch changes the driver back to useing kzalloc() and
>> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
>> causes an oops on memcpy()[1]. We are in the process of bisecting that
>> one now and will provide the results.
> The two bugs are the same it's that the code has shifted a little. Mark
> the commit as buggy and continue with the git bisect.
>
> regards,
> dan carpenter
Can you explain a little further? Mark commit a4a23f6 as bad? An
initial bisect already reported that was the first bad commit, so it
can't be marked bad. The oops on memcpy() happens after commit a4a23f6
is reverted. The oops on memcpy() did not happen before a4a23f6 was
committed, so I assume this new oops was introduced by a change later.
Right now I'm bisecting down the oops on memcpy() by updating the bisect
with good or bad, depending if the test kernel hit the oops. I then
revert a4a23f6, so that revert is the HEAD of the tree each time before
building the kernel again(As long as the commit spit out by bisect is
after when a4a23f6 was introduced).
Thanks,
Joe
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Dan Carpenter @ 2013-09-16 21:05 UTC (permalink / raw)
To: Joseph Salisbury
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <52376ED9.5080208@canonical.com>
On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> > On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> >> Reverting the patch changes the driver back to useing kzalloc() and
> >> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> >> causes an oops on memcpy()[1]. We are in the process of bisecting that
> >> one now and will provide the results.
> > The two bugs are the same it's that the code has shifted a little. Mark
> > the commit as buggy and continue with the git bisect.
> >
> > regards,
> > dan carpenter
> Can you explain a little further? Mark commit a4a23f6 as bad? An
> initial bisect already reported that was the first bad commit, so it
> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
> is reverted. The oops on memcpy() did not happen before a4a23f6 was
> committed, so I assume this new oops was introduced by a change later.
>
> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> with good or bad, depending if the test kernel hit the oops. I then
> revert a4a23f6, so that revert is the HEAD of the tree each time before
> building the kernel again(As long as the commit spit out by bisect is
> after when a4a23f6 was introduced).
Yep. Please continue bisecting the memcpy() oops.
kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
apart by reverting the patch then we would expect the oops to move to
the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
immediately see how that is possible.
regards,
dan carpenter
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 21:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916201328.GN19256@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 1:13 PM
> To: KY Srinivasan
> Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; Dmitry
> Torokhov; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote:
> > Dan,
> >
> > Rolling the changes you have indicated is not the issue; this can trivially be
> done.
> > My contention is that it is not needed given that the underlying function is
> already
> > doing that. Look at the function vmbus_recvpacket_raw() in
> drivers/hv/channel.c.
> >
>
> I'm confused.
>
> There is no mention of ->offset8 in vmbus_recvpacket_raw().
As you can see the vmbus_recvpacket_raw() ensures that the complete
packet is read and if the buffer specified is not large enough nothing is
read. The packet header has information about the length of the packet
and the offset where the payload is.
>
> It's a good idea to add a check there but the lower levels don't know
> about the sizeof(synth_kbd_protocol_response) so we would still need
> something like my check.
Why would the lower level code need to know anything about the layout of a
particular message type. The lower level code is guaranteeing that a complete
packet has been read in and that should be sufficient - assuming we trust the host.
We have already spent more time on this than we should; I will make the necessary
changes.
Regards,
K. Y
>
> regards,
> dan carpenter
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 22:13 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <a76e49ef4b7f44f9a99acea5295d1491@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 09:55:44PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, September 16, 2013 1:13 PM
> > To: KY Srinivasan
> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; Dmitry
> > Torokhov; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> > input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote:
> > > Dan,
> > >
> > > Rolling the changes you have indicated is not the issue; this can trivially be
> > done.
> > > My contention is that it is not needed given that the underlying function is
> > already
> > > doing that. Look at the function vmbus_recvpacket_raw() in
> > drivers/hv/channel.c.
> > >
> >
> > I'm confused.
> >
> > There is no mention of ->offset8 in vmbus_recvpacket_raw().
>
> As you can see the vmbus_recvpacket_raw() ensures that the complete
> packet is read and if the buffer specified is not large enough nothing is
> read. The packet header has information about the length of the packet
> and the offset where the payload is.
> >
No one is talking about the ->len8. I'm saying that we should check
->offset8.
> > It's a good idea to add a check there but the lower levels don't know
> > about the sizeof(synth_kbd_protocol_response) so we would still need
> > something like my check.
>
> Why would the lower level code need to know anything about the layout of a
> particular message type. The lower level code is guaranteeing that a complete
> packet has been read in and that should be sufficient - assuming we trust the host.
>
Of course, we don't need to check anything if we trust the host. I said
that already. Just add the check for robustness.
> We have already spent more time on this than we should; I will make the necessary
> changes.
Thank you.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 22:16 UTC (permalink / raw)
To: KY Srinivasan
Cc: Dan Carpenter, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <3e45f054c6e243eabf2896ab1dcf38d4@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 06:29:45PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Monday, September 16, 2013 10:10 AM
> > To: KY Srinivasan
> > Cc: Dan Carpenter; olaf@aepfle.de; gregkh@linuxfoundation.org;
> > jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> > input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > > Sent: Monday, September 16, 2013 8:06 AM
> > > > To: KY Srinivasan
> > > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > > > dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org;
> > vojtech@suse.cz;
> > > > linux-input@vger.kernel.org; apw@canonical.com;
> > devel@linuxdriverproject.org
> > > > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > > > synthetic keyboard
> > > >
> > > > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > > > + case VM_PKT_DATA_INBAND:
> > > > > > > + hv_kbd_on_receive(device, desc);
> > > > > >
> > > > > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > > > > doesn't take into consideration the amount of data we recieved, it
> > > > > > trusts the offset we recieved from the user. There is an out of bounds
> > > > > > read.
> > > > >
> > > > > What user are you referring to. The message is sent by the host - the user
> > > > keystroke
> > > > > is normalized into a fixed size packet by the host and sent to the guest. We
> > will
> > > > parse this
> > > > > packet, based on the host specified layout here.
> > > > >
> > > >
> > > > The user means the hypervisor, yes.
> > > >
> > > > I don't want the hypervisor accessing outside of the buffer. It is
> > > > robustness issue. Just check the offset against "bytes_recvd". It's
> > > > not complicated.
> > >
> > > At the outset, let me apologize for not understanding your concern.
> > > You say: " I don't want the hypervisor accessing outside of the buffer"
> > > Where did you see the hypervisor accessing anything outside the buffer?
> > > The buffer is allocated by this driver and a packet from vmbus is read into this
> > > buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> > > than the packet that needs to be read, then nothing will be read. Once the read
> > > completes, we can be sure we have read a valid packet and can proceed to
> > parse it in
> > > this driver.
> >
> > The concern is that number of bytes received and contents of a packet
> > are not in sync. Imagine if we were told that 16 butes was received but
> > in the packet offset is 78. Then we'll try reading well past the buffer
> > boundary that we allocated for the packets.
>
> I am not sure how this would be the case. Following are the semantics of the function
> vmbus_recvpacket_raw():
>
> If the packet to be read is larger than the buffer specified; nothing will be read and
> appropriate error is returned. If a packet is read, the complete packet is read and
> so we can safely peek into this packet based on the information in the header.
No, you can not safely use contents of the packet because it has not
been vetted. The semantics you are talking about is provided by
vmbus_recvpacket(). That function does indeed look inside the packet end
ensures that offset specified in the packet is sane and would not exceed
the buffer. The vmbus_recvpacket_raw() does not do such validation.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Joseph Salisbury @ 2013-09-17 0:44 UTC (permalink / raw)
To: Dan Carpenter
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <20130916210503.GQ25896@mwanda>
On 09/16/2013 05:05 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
>> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
>>> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>>>> Reverting the patch changes the driver back to useing kzalloc() and
>>>> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
>>>> causes an oops on memcpy()[1]. We are in the process of bisecting that
>>>> one now and will provide the results.
>>> The two bugs are the same it's that the code has shifted a little. Mark
>>> the commit as buggy and continue with the git bisect.
>>>
>>> regards,
>>> dan carpenter
>> Can you explain a little further? Mark commit a4a23f6 as bad? An
>> initial bisect already reported that was the first bad commit, so it
>> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
>> is reverted. The oops on memcpy() did not happen before a4a23f6 was
>> committed, so I assume this new oops was introduced by a change later.
>>
>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>> with good or bad, depending if the test kernel hit the oops. I then
>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>> building the kernel again(As long as the commit spit out by bisect is
>> after when a4a23f6 was introduced).
> Yep. Please continue bisecting the memcpy() oops.
>
> kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
> apart by reverting the patch then we would expect the oops to move to
> the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
> immediately see how that is possible.
>
> regards,
> dan carpenter
Thanks for the details. We'll continue the bisect and let you know how
it goes.
Thanks again,
Joe
^ permalink raw reply
* [PATCH V2] Input: atmel_mxt_ts - update to devm_* API
From: Manish Badarkhe @ 2013-09-17 3:20 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: nick.dyer, dmitry.torokhov, josh.wu
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
Changes since V1:
As per Dmitry's comment, avoid "devm_" operation on irq as it may cause
kernel crash while device is getting unbound and CCing Nick dryer.
:100644 100644 59aa240... d04dbed... M drivers/input/touchscreen/atmel_mxt_ts.c
drivers/input/touchscreen/atmel_mxt_ts.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 59aa240..d04dbed 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1139,12 +1139,12 @@ static int mxt_probe(struct i2c_client *client,
if (!pdata)
return -EINVAL;
- data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
- input_dev = input_allocate_device();
+ data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
+ input_dev = devm_input_allocate_device(&client->dev);
if (!data || !input_dev) {
dev_err(&client->dev, "Failed to allocate memory\n");
error = -ENOMEM;
- goto err_free_mem;
+ goto err_out;
}
data->is_tp = pdata && pdata->is_tp;
@@ -1170,7 +1170,7 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_initialize(data);
if (error)
- goto err_free_mem;
+ goto err_out;
__set_bit(EV_ABS, input_dev->evbit);
__set_bit(EV_KEY, input_dev->evbit);
@@ -1253,9 +1253,7 @@ err_free_irq:
free_irq(client->irq, data);
err_free_object:
kfree(data->object_table);
-err_free_mem:
- input_free_device(input_dev);
- kfree(data);
+err_out:
return error;
}
@@ -1267,7 +1265,6 @@ static int mxt_remove(struct i2c_client *client)
free_irq(data->irq, data);
input_unregister_device(data->input_dev);
kfree(data->object_table);
- kfree(data);
return 0;
}
--
1.7.10.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox