* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Alan Stern @ 2019-08-22 14:49 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Oliver Neukum, jikos, benjamin.tissoires, linux-input,
linux-kernel, linux-usb
In-Reply-To: <D6E31CB0-BC2B-4B52-AF18-4BE990D3FDA5@canonical.com>
On Thu, 22 Aug 2019, Kai-Heng Feng wrote:
> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
>
> > Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
> >> Hi Oliver,
> >>
> >> at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
> >>
> >>> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
> >>>> The optical sensor of the mouse gets turned off when it's runtime
> >>>> suspended, so moving the mouse can't wake the mouse up, despite that
> >>>> USB remote wakeup is successfully set.
> >>>>
> >>>> Introduce a new quirk to prevent the mouse from getting runtime
> >>>> suspended.
> >>>
> >>> Hi,
> >>>
> >>> I am afraid this is a bad approach in principle. The device
> >>> behaves according to spec.
> >>
> >> Can you please point out which spec it is? Is it USB 2.0 spec?
> >
> > Well, sort of. The USB spec merely states how to enter and exit
> > a suspended state and that device state must not be lost.
> > It does not tell you what a suspended device must be able to do.
>
> But shouldn’t remote wakeup signaling wakes the device up and let it exit
> suspend state?
> Or it’s okay to let the device be suspended when remote wakeup is needed
> but broken?
>
> >
> >>> And it behaves like most hardware.
> >>
> >> So seems like most hardware are broken.
> >> Maybe a more appropriate solution is to disable RPM for all USB mice.
> >
> > That is a decision a distro certainly can make. However, the kernel
> > does not, by default, call usb_enable_autosuspend() for HID devices
> > for this very reason. It is enabled by default only for hubs,
> > BT dongles and UVC cameras (and some minor devices)
> >
> > In other words, if on your system it is on, you need to look
> > at udev, not the kernel.
>
> So if a device is broken when “power/control” is flipped by user, we should
> deal it at userspace? That doesn’t sound right to me.
>
> >
> >>> If you do not want runtime PM for such devices, do not switch
> >>> it on.
> >>
> >> A device should work regardless of runtime PM status.
> >
> > Well, no. Runtime PM is a trade off. You lose something if you use
> > it. If it worked just as well as full power, you would never use
> > full power, would you?
>
> I am not asking the suspended state to work as full power, but to prevent a
> device enters suspend state because of broken remote wakeup.
>
> >
> > Whether the loss of functionality or performance is worth the energy
> > savings is a policy decision. Hence it belongs into udev.
> > Ideally the kernel would tell user space what will work in a
> > suspended state. Unfortunately HID does not provide support for that.
>
> I really don’t think “loss of functionally” belongs to policy decision. But
> that’s just my opinion.
>
> >
> > This is a deficiency of user space. The kernel has an ioctl()
> > to let user space tell it, whether a device is fully needed.
> > X does not use them.
>
> Ok, I’ll take a look at other device drivers that use it.
>
> >
> >>> The refcounting needs to be done correctly.
> >>
> >> Will do.
> >
> > Well, I am afraid your patch breaks it and if you do not break
> > it, the patch is reduced to nothing.
>
> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
> the refcount?
>
> >
> >>> This patch does something that udev should do and in a
> >>> questionable manner.
> >>
> >> IMO if the device doesn’t support runtime suspend, then it needs to be
> >> disabled in kernel but not workaround in userspace.
> >
> > You switch it on from user space. Of course the kernel default
> > must be safe, as you said. It already is.
>
> I’d also like to hear maintainers' opinion on this issue.
I agree with Oliver. There is no formal requirement on what actions
should cause a mouse to generate a remote wakeup request. Some mice
will do it when they are moved and some mice won't.
If you don't like the way a particular mouse behaves then you should
not allow it to go into runtime suspend. By default, the kernel
prevents _all_ USB mice from being runtime suspended; the only way a
mouse can be suspended is if some userspace program tells the kernel to
allow it.
It might be a udev script which does this, or a powertop setting, or
something else. Regardless, what the kernel does is correct.
Furthermore, the kernel has to accomodate users who don't mind pressing
a mouse button to wake up their mice. For their sake, the kernel
should not forbid a mouse from ever going into runtime suspend merely
because it won't generate a wakeup request when it is moved.
Alan Stern
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Oliver Neukum @ 2019-08-22 13:37 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, linux-usb
In-Reply-To: <D6E31CB0-BC2B-4B52-AF18-4BE990D3FDA5@canonical.com>
Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng:
> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
> > Well, sort of. The USB spec merely states how to enter and exit
> > a suspended state and that device state must not be lost.
> > It does not tell you what a suspended device must be able to do.
>
> But shouldn’t remote wakeup signaling wakes the device up and let it exit
> suspend state?
Yes. Have you tested using a button? If they indeed do not work, then
the device lies about supporting remote wakeup. That would warrant a
quirk, but for remote wakeup.
> Or it’s okay to let the device be suspended when remote wakeup is needed
> but broken?
Again, the HID spec does not specify what should trigger a remote
wakeup. Limiting this to mouse buttons but not movements is
inconvinient, but not buggy.
This is indeed what Windows does. The device is suspended when the
screen saver switches on. That we do not do that is a deficiency
of X.
To use runtime PM regularly you need an .ini file
> > In other words, if on your system it is on, you need to look
> > at udev, not the kernel.
>
> So if a device is broken when “power/control” is flipped by user, we should
> deal it at userspace? That doesn’t sound right to me.
If it is broken, as in crashing we could talk about it. If it merely
does not do what you want, then, yes, that is for user space to deal
with.
> > Well, no. Runtime PM is a trade off. You lose something if you use
> > it. If it worked just as well as full power, you would never use
> > full power, would you?
>
> I am not asking the suspended state to work as full power, but to prevent a
> device enters suspend state because of broken remote wakeup.
What then would be the difference between suspended and active? A small
delay in data transfer?
> > Whether the loss of functionality or performance is worth the energy
> > savings is a policy decision. Hence it belongs into udev.
> > Ideally the kernel would tell user space what will work in a
> > suspended state. Unfortunately HID does not provide support for that.
>
> I really don’t think “loss of functionally” belongs to policy decision. But
> that’s just my opinion.
That is just what we do if, for example, you choose between the configs
of a USB device or when you use authorization.
> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
> the refcount?
No, the refcount is good. If remote wakeup is totally broken, you need
to introduce a quirk that will prevent the kernel from believing the
device when it claims to support it.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Kai-Heng Feng @ 2019-08-22 13:23 UTC (permalink / raw)
To: Oliver Neukum
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, linux-usb
In-Reply-To: <1566470325.8347.35.camel@suse.com>
at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
> Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
>> Hi Oliver,
>>
>> at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
>>
>>> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
>>>> The optical sensor of the mouse gets turned off when it's runtime
>>>> suspended, so moving the mouse can't wake the mouse up, despite that
>>>> USB remote wakeup is successfully set.
>>>>
>>>> Introduce a new quirk to prevent the mouse from getting runtime
>>>> suspended.
>>>
>>> Hi,
>>>
>>> I am afraid this is a bad approach in principle. The device
>>> behaves according to spec.
>>
>> Can you please point out which spec it is? Is it USB 2.0 spec?
>
> Well, sort of. The USB spec merely states how to enter and exit
> a suspended state and that device state must not be lost.
> It does not tell you what a suspended device must be able to do.
But shouldn’t remote wakeup signaling wakes the device up and let it exit
suspend state?
Or it’s okay to let the device be suspended when remote wakeup is needed
but broken?
>
>>> And it behaves like most hardware.
>>
>> So seems like most hardware are broken.
>> Maybe a more appropriate solution is to disable RPM for all USB mice.
>
> That is a decision a distro certainly can make. However, the kernel
> does not, by default, call usb_enable_autosuspend() for HID devices
> for this very reason. It is enabled by default only for hubs,
> BT dongles and UVC cameras (and some minor devices)
>
> In other words, if on your system it is on, you need to look
> at udev, not the kernel.
So if a device is broken when “power/control” is flipped by user, we should
deal it at userspace? That doesn’t sound right to me.
>
>>> If you do not want runtime PM for such devices, do not switch
>>> it on.
>>
>> A device should work regardless of runtime PM status.
>
> Well, no. Runtime PM is a trade off. You lose something if you use
> it. If it worked just as well as full power, you would never use
> full power, would you?
I am not asking the suspended state to work as full power, but to prevent a
device enters suspend state because of broken remote wakeup.
>
> Whether the loss of functionality or performance is worth the energy
> savings is a policy decision. Hence it belongs into udev.
> Ideally the kernel would tell user space what will work in a
> suspended state. Unfortunately HID does not provide support for that.
I really don’t think “loss of functionally” belongs to policy decision. But
that’s just my opinion.
>
> This is a deficiency of user space. The kernel has an ioctl()
> to let user space tell it, whether a device is fully needed.
> X does not use them.
Ok, I’ll take a look at other device drivers that use it.
>
>>> The refcounting needs to be done correctly.
>>
>> Will do.
>
> Well, I am afraid your patch breaks it and if you do not break
> it, the patch is reduced to nothing.
Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
the refcount?
>
>>> This patch does something that udev should do and in a
>>> questionable manner.
>>
>> IMO if the device doesn’t support runtime suspend, then it needs to be
>> disabled in kernel but not workaround in userspace.
>
> You switch it on from user space. Of course the kernel default
> must be safe, as you said. It already is.
I’d also like to hear maintainers' opinion on this issue.
Kai-Heng
>
> Regards
> Oliver
^ permalink raw reply
* Re: [PATCH] driver:st1633: fixed multitouch incorrect coordinates
From: Martin Kepplinger @ 2019-08-22 12:08 UTC (permalink / raw)
To: Dixit Parmar
Cc: dmitry.torokhov, rydberg, kuninori.morimoto.gx, robh,
matthias.fend, linux-input, linux-kernel
In-Reply-To: <1566209314-21767-1-git-send-email-dixitparmar19@gmail.com>
Am 19.08.2019 12:08 schrieb Dixit Parmar:
> From: Dixit Parmar <dixitparmar19@gmail.com>
>
> For Sitronix st1633 multi-touch controller driver the co-ordinates
> reported
> for multiple fingers were wrong.
>
> So the below mentioned bug was filed,
> Bugzilla Bug ID: 204561
>
> While reading co-ordinates from specified I2C registers, the X & Y
> co-ordinates should be read from proper I2C address for particular
> finger as
> specified in chip specific datasheet.
>
> for single touch this logic is working fine. However, for multi-touch
> scenario the logic of reading data from data buffer has issues.
>
> This patch fixes the reading logic from data buffer.
>
> Previous logic:
> * Offset of X & Y Lower byte coordinate is increased by i no. only(by 1
> Byte)
> for each finger.
>
> New logic:
> * The logic of reading X & Y Lower Byte coordinate needs to be
> increased
> by i+y for each time/finger.
>
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
> drivers/input/touchscreen/st1232.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/st1232.c
> b/drivers/input/touchscreen/st1232.c
> index 3492339..1139714 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -81,8 +81,10 @@ static int st1232_ts_read_data(struct st1232_ts_data
> *ts)
> for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> finger[i].is_valid = buf[i + y] >> 7;
> if (finger[i].is_valid) {
> - finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> - finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
> + finger[i].x = ((buf[i + y] & 0x0070) << 4) |
> + buf[i + y + 1];
> + finger[i].y = ((buf[i + y] & 0x0007) << 8) |
> + buf[i + y + 2];
Seems like you're right. It's simply +1 (for x) and +2 (for y) from the
high-byte locations.
Not sure how that went wrong.
Thank you,
Reviewed-by: Martin Kepplinger <martink@posteo.de>
>
> /* st1232 includes a z-axis / touch strength */
> if (ts->chip_info->have_z)
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Oliver Neukum @ 2019-08-22 10:38 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, linux-usb
In-Reply-To: <AD8A4135-0275-45B3-BEB9-031737A2C756@canonical.com>
Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
> Hi Oliver,
>
> at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
>
> > Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
> > > The optical sensor of the mouse gets turned off when it's runtime
> > > suspended, so moving the mouse can't wake the mouse up, despite that
> > > USB remote wakeup is successfully set.
> > >
> > > Introduce a new quirk to prevent the mouse from getting runtime
> > > suspended.
> >
> > Hi,
> >
> > I am afraid this is a bad approach in principle. The device
> > behaves according to spec.
>
> Can you please point out which spec it is? Is it USB 2.0 spec?
Well, sort of. The USB spec merely states how to enter and exit
a suspended state and that device state must not be lost.
It does not tell you what a suspended device must be able to do.
> > And it behaves like most hardware.
>
> So seems like most hardware are broken.
> Maybe a more appropriate solution is to disable RPM for all USB mice.
That is a decision a distro certainly can make. However, the kernel
does not, by default, call usb_enable_autosuspend() for HID devices
for this very reason. It is enabled by default only for hubs,
BT dongles and UVC cameras (and some minor devices)
In other words, if on your system it is on, you need to look
at udev, not the kernel.
> > If you do not want runtime PM for such devices, do not switch
> > it on.
>
> A device should work regardless of runtime PM status.
Well, no. Runtime PM is a trade off. You lose something if you use
it. If it worked just as well as full power, you would never use
full power, would you?
Whether the loss of functionality or performance is worth the energy
savings is a policy decision. Hence it belongs into udev.
Ideally the kernel would tell user space what will work in a
suspended state. Unfortunately HID does not provide support for that.
This is a deficiency of user space. The kernel has an ioctl()
to let user space tell it, whether a device is fully needed.
X does not use them.
> > The refcounting needs to be done correctly.
>
> Will do.
Well, I am afraid your patch breaks it and if you do not break
it, the patch is reduced to nothing.
> >
> > This patch does something that udev should do and in a
> > questionable manner.
>
> IMO if the device doesn’t support runtime suspend, then it needs to be
> disabled in kernel but not workaround in userspace.
You switch it on from user space. Of course the kernel default
must be safe, as you said. It already is.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Kai-Heng Feng @ 2019-08-22 10:04 UTC (permalink / raw)
To: Oliver Neukum
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, linux-usb
In-Reply-To: <1566467151.8347.23.camel@suse.com>
Hi Oliver,
at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
>> The optical sensor of the mouse gets turned off when it's runtime
>> suspended, so moving the mouse can't wake the mouse up, despite that
>> USB remote wakeup is successfully set.
>>
>> Introduce a new quirk to prevent the mouse from getting runtime
>> suspended.
>
> Hi,
>
> I am afraid this is a bad approach in principle. The device
> behaves according to spec.
Can you please point out which spec it is? Is it USB 2.0 spec?
> And it behaves like most hardware.
So seems like most hardware are broken.
Maybe a more appropriate solution is to disable RPM for all USB mice.
> If you do not want runtime PM for such devices, do not switch
> it on.
A device should work regardless of runtime PM status.
> The refcounting needs to be done correctly.
Will do.
>
> This patch does something that udev should do and in a
> questionable manner.
IMO if the device doesn’t support runtime suspend, then it needs to be
disabled in kernel but not workaround in userspace.
Kai-Heng
>
> Regards
> Oliver
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Oliver Neukum @ 2019-08-22 9:45 UTC (permalink / raw)
To: Kai-Heng Feng, jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, linux-usb
In-Reply-To: <20190822091744.3451-1-kai.heng.feng@canonical.com>
Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
> The optical sensor of the mouse gets turned off when it's runtime
> suspended, so moving the mouse can't wake the mouse up, despite that
> USB remote wakeup is successfully set.
>
> Introduce a new quirk to prevent the mouse from getting runtime
> suspended.
Hi,
I am afraid this is a bad approach in principle. The device
behaves according to spec. And it behaves like most hardware.
If you do not want runtime PM for such devices, do not switch
it on. The refcounting needs to be done correctly.
This patch does something that udev should do and in a
questionable manner.
Regards
Oliver
^ permalink raw reply
* [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Kai-Heng Feng @ 2019-08-22 9:17 UTC (permalink / raw)
To: jikos, benjamin.tissoires
Cc: linux-input, linux-usb, linux-kernel, Kai-Heng Feng
The optical sensor of the mouse gets turned off when it's runtime
suspended, so moving the mouse can't wake the mouse up, despite that
USB remote wakeup is successfully set.
Introduce a new quirk to prevent the mouse from getting runtime
suspended.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/hid/hid-quirks.c | 2 +-
drivers/hid/usbhid/hid-core.c | 3 ++-
include/linux/hid.h | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 166f41f3173b..40574f856a93 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -108,7 +108,7 @@ static const struct hid_device_id hid_quirks[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C05A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C06A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_GAMEPADBLOCK), HID_QUIRK_MULTI_INPUT },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_ALWAYS_POLL },
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_NO_RUNTIME_PM },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2), HID_QUIRK_NO_INIT_REPORTS },
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index c7bc9db5b192..08a6b4f5cfb2 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -713,7 +713,8 @@ static int usbhid_open(struct hid_device *hid)
}
}
- usb_autopm_put_interface(usbhid->intf);
+ if (!(hid->quirks & HID_QUIRK_NO_RUNTIME_PM))
+ usb_autopm_put_interface(usbhid->intf);
/*
* In case events are generated while nobody was listening,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d770ab1a0479..bec413226146 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -342,6 +342,7 @@ struct hid_item {
/* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
#define HID_QUIRK_ALWAYS_POLL BIT(10)
#define HID_QUIRK_INPUT_PER_APP BIT(11)
+#define HID_QUIRK_NO_RUNTIME_PM BIT(12)
#define HID_QUIRK_SKIP_OUTPUT_REPORTS BIT(16)
#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID BIT(17)
#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP BIT(18)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v1 39/63] Input: touchscreen: Atmel: Add device tree support for T15 key array objects
From: Jiada Wang @ 2019-08-22 7:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816172500.GI121898@dtor-ws>
Hi
On 2019/08/17 2:25, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:35:01PM +0900, Jiada Wang wrote:
>> From: Daniel Gong <Zhanli.Gong@cn.bosch.com>
>
> This should be with the code adding T15 handling.
>
will squash this patch into T15 handling one in v2 patchset
Thanks,
Jiada
>>
>> Signed-off-by: Daniel Gong <Zhanli.Gong@cn.bosch.com>
>> Signed-off-by: George G. Davis <george_davis@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>> drivers/input/touchscreen/atmel_mxt_ts.c | 29 ++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index be63002c2b31..3b9544c0a209 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -4143,10 +4143,12 @@ static int mxt_parse_device_properties(struct mxt_data *data)
>> {
>> static const char keymap_property[] = "linux,gpio-keymap";
>> static const char gpios_property[] = "atmel,gpios";
>> + static const char buttons_property[] = "atmel,key-buttons";
>> struct device *dev = &data->client->dev;
>> struct device_node *np = dev ? dev->of_node : NULL;
>> struct device_node *np_gpio;
>> u32 *keymap;
>> + u32 *buttonmap;
>> int n_keys;
>> int error;
>>
>> @@ -4181,6 +4183,33 @@ static int mxt_parse_device_properties(struct mxt_data *data)
>> data->t19_num_keys = n_keys;
>> }
>>
>> + if (device_property_present(dev, buttons_property)) {
>> + n_keys = device_property_read_u32_array(dev, buttons_property,
>> + NULL, 0);
>> + if (n_keys <= 0) {
>> + error = n_keys < 0 ? n_keys : -EINVAL;
>> + dev_err(dev, "invalid/malformed '%s' property: %d\n",
>> + buttons_property, error);
>> + return error;
>> + }
>> +
>> + buttonmap = devm_kmalloc_array(dev, n_keys, sizeof(*buttonmap),
>> + GFP_KERNEL);
>> + if (!buttonmap)
>> + return -ENOMEM;
>> +
>> + error = device_property_read_u32_array(dev, buttons_property,
>> + buttonmap, n_keys);
>> + if (error) {
>> + dev_err(dev, "failed to parse '%s' property: %d\n",
>> + buttons_property, error);
>> + return error;
>> + }
>> +
>> + data->t15_keymap = buttonmap;
>> + data->t15_num_keys = n_keys;
>> + }
>> +
>> device_property_read_u32(dev, "atmel,suspend-mode", &data->suspend_mode);
>>
>> np_gpio = of_get_child_by_name(np, gpios_property);
>> --
>> 2.19.2
>>
>
^ permalink raw reply
* Re: [PATCH] HID: hidraw: Fix invalid read in hidraw_ioctl
From: Jiri Kosina @ 2019-08-22 7:51 UTC (permalink / raw)
To: Alan Stern
Cc: andreyknvl, benjamin.tissoires, linux-input,
Kernel development list, USB list, syzkaller-bugs
In-Reply-To: <Pine.LNX.4.44L0.1908211323030.1816-100000@iolanthe.rowland.org>
On Wed, 21 Aug 2019, Alan Stern wrote:
> The syzbot fuzzer has reported a pair of problems in the
> hidraw_ioctl() function: slab-out-of-bounds read and use-after-free
> read. An example of the first:
>
> BUG: KASAN: slab-out-of-bounds in strlen+0x79/0x90 lib/string.c:525
> Read of size 1 at addr ffff8881c8035f38 by task syz-executor.4/2833
>
> CPU: 1 PID: 2833 Comm: syz-executor.4 Not tainted 5.3.0-rc2+ #1
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> print_address_description+0x6a/0x32c mm/kasan/report.c:351
> __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:612
> strlen+0x79/0x90 lib/string.c:525
> strlen include/linux/string.h:281 [inline]
> hidraw_ioctl+0x245/0xae0 drivers/hid/hidraw.c:446
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:509 [inline]
> do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
> ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
> __do_sys_ioctl fs/ioctl.c:720 [inline]
> __se_sys_ioctl fs/ioctl.c:718 [inline]
> __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x459829
> Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f7a68f6dc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000459829
> RDX: 0000000000000000 RSI: 0000000080404805 RDI: 0000000000000004
> RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f7a68f6e6d4
> R13: 00000000004c21de R14: 00000000004d5620 R15: 00000000ffffffff
>
> The two problems have the same cause: hidraw_ioctl() fails to test
> whether the device has been removed. This patch adds the missing test.
>
> Reported-and-tested-by: syzbot+5a6c4ec678a0c6ee84ba@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Thanks a lot Alan for chasing this; I've applied the patch.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v1 41/63] Input: touchscreen: Atmel: Enable IRQ_DISABLE_UNLAZY flag for interrupt
From: Jiada Wang @ 2019-08-22 6:25 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816172615.GJ121898@dtor-ws>
Hi Dmitry
On 2019/08/17 2:26, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:35:36PM +0900, Jiada Wang wrote:
>> From: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
>>
>> The de-/serializer driver has defined only irq_mask "ds90ub927_irq_mask" and
>> irq_unmask "ds90ub927_irq_unmask" callback functions. And de-/serializer
>> driver doesn't implement the irq_disable and irq_enable callback functions.
>> Hence inorder to invoke irq_mask callback function when disable_irq_nosync is
>> called the IRQ_DISABLE_UNLAZY interrupt flag should be set. If not the
>> disable_irq_nosync will just increment the depth field in the irq
>> descriptor only once as shown below.
>>
>> disable_irq_nosync
>> __disable_irq_nosync
>> __disable_irq (desc->depth++)
>> irq_disable
>> if irq_disable present -----------> if IRQ_DISABLE_UNLAZYflag set
>> | no |
>> yes | yes |
>> | |
>> desc->irq_data.chip->irq_disable desc->irq_data.chip->irq_unmask
>> (ds90ub927_irq_mask)
>> disable_irq
>> __disable_irq_nosync
>> __disable_irq
>> (desc->depth++)
>> But the enable_irq will try to decrement the depth field twice which generates
>> the backtrace stating "Unbalanced enable for irq 293". This is because there is
>> no IRQ_DISABLE_UNLAZY flag check while calling irq_unmask callback function
>> of the "ds90ub927_irq_unmask" de-/serializer via enable_irq.
>>
>> enable_irq
>> __enable_irq (desc->depth--)
>> irq_enable
>> if irq_enable present -------------> desc->irq_data.chip->irq_unmask
>> | no (ds90ub927_irq_unmask)
>> yes | enable_irq
>> | __enable_irq (desc->depth--)
>> (desc->irq_data.chip->irq_enable)
>
> I'd prefer if we instead did not use the disable_irq_nosync() in the
> driver.
>
sorry for the mistake, during forward port,
I have already eliminated disable_irq_nosync(),
so this patch is no longer needed,
will drop it in v2 patch-set
Thanks，
Jiada
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v1 06/63] Input: atmel_mxt_ts - output status from T42 Touch Suppression
From: Jiada Wang @ 2019-08-22 5:52 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816173426.GM121898@dtor-ws>
Hi
On 2019/08/17 2:34, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:30:33PM +0900, Jiada Wang wrote:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
>> Acked-by: Benson Leung <bleung@chromium.org>
>> Acked-by: Yufeng Shen <miletus@chromium.org>
>> (cherry picked from ndyer/linux/for-upstream commit ab95b5a309999d2c098daaa9f88d9fcfae7eb516)
>> Signed-off-by: George G. Davis <george_davis@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>> drivers/input/touchscreen/atmel_mxt_ts.c | 25 ++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index a75c35c6f9f9..9226ec528adf 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -155,6 +155,9 @@ struct t37_debug {
>> #define MXT_RESET_VALUE 0x01
>> #define MXT_BACKUP_VALUE 0x55
>>
>> +/* Define for MXT_PROCI_TOUCHSUPPRESSION_T42 */
>> +#define MXT_T42_MSG_TCHSUP BIT(0)
>> +
>> /* T100 Multiple Touch Touchscreen */
>> #define MXT_T100_CTRL 0
>> #define MXT_T100_CFG1 1
>> @@ -323,6 +326,8 @@ struct mxt_data {
>> u8 T9_reportid_max;
>> u16 T18_address;
>> u8 T19_reportid;
>> + u8 T42_reportid_min;
>> + u8 T42_reportid_max;
>> u16 T44_address;
>> u8 T48_reportid;
>> u8 T100_reportid_min;
>> @@ -978,6 +983,17 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
>> data->update_input = true;
>> }
>>
>> +static void mxt_proc_t42_messages(struct mxt_data *data, u8 *msg)
>> +{
>> + struct device *dev = &data->client->dev;
>> + u8 status = msg[1];
>> +
>> + if (status & MXT_T42_MSG_TCHSUP)
>> + dev_info(dev, "T42 suppress\n");
>> + else
>> + dev_info(dev, "T42 normal\n");
>
> dev_dbg(). There is no need to flood the logs with this. I'd assume this
> is for assisting in bringup. Should there be some more generic way of
> monitoring the status?
>
will replace with dev_dbg() in v2 patchset
thanks,
Jiada
^ permalink raw reply
* Re: [PATCH v1 04/63] Input: atmel_mxt_ts - split large i2c transfers into blocks
From: Jiada Wang @ 2019-08-22 5:24 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190816171837.GG121898@dtor-ws>
Hi Dmitry
On 2019/08/17 2:18, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:28:53PM +0900, Jiada Wang wrote:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> On some firmware variants, the size of the info block exceeds what can
>> be read in a single transfer.
>>
>> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
>> (cherry picked from ndyer/linux/for-upstream commit 74c4f5277cfa403d43fafc404119dc57a08677db)
>> [gdavis: Forward port and fix conflicts due to v4.14.51 commit
>> 960fe000b1d3 ("Input: atmel_mxt_ts - fix the firmware
>> update").]
>> Signed-off-by: George G. Davis <george_davis@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>> drivers/input/touchscreen/atmel_mxt_ts.c | 27 +++++++++++++++++++++---
>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 9b165d23e092..2d70ddf71cd9 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -40,7 +40,7 @@
>> #define MXT_OBJECT_START 0x07
>> #define MXT_OBJECT_SIZE 6
>> #define MXT_INFO_CHECKSUM_SIZE 3
>> -#define MXT_MAX_BLOCK_WRITE 256
>> +#define MXT_MAX_BLOCK_WRITE 255
>>
>> /* Object types */
>> #define MXT_DEBUG_DIAGNOSTIC_T37 37
>> @@ -659,6 +659,27 @@ static int __mxt_read_reg(struct i2c_client *client,
>> return ret;
>> }
>>
>> +static int mxt_read_blks(struct mxt_data *data, u16 start, u16 count, u8 *buf)
>
> Can we call this __mxt_read_reg() and the original read reg call
> __mxt_read_chunk()?
>
yes, I will update in v2 patch-set,
so that every call to __mxt_read_reg() in atmel driver,
can have the feature to split large size transfer.
Thanks,
Jiada
>> +{
>> + u16 offset = 0;
>> + int error;
>> + u16 size;
>> +
>> + while (offset < count) {
>> + size = min(MXT_MAX_BLOCK_WRITE, count - offset);
>> +
>> + error = __mxt_read_reg(data->client,
>> + start + offset,
>> + size, buf + offset);
>> + if (error)
>> + return error;
>> +
>> + offset += size;
>> + }
>> +
>> + return 0;
>> +}
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v1 03/63] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary
From: Jiada Wang @ 2019-08-22 3:37 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <20190821175422.GE76194@dtor-ws>
Hi
On 2019/08/22 2:54, Dmitry Torokhov wrote:
> On Wed, Aug 21, 2019 at 10:26:31PM +0900, Jiada Wang wrote:
>> Hi Dmitry
>>
>> On 2019/08/17 2:16, Dmitry Torokhov wrote:
>>> On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>>>
>>>> The workaround of reading all messages until an invalid is received is a
>>>> way of forcing the CHG line high, which means that when using
>>>> edge-triggered interrupts the interrupt can be acquired.
>>>>
>>>> With level-triggered interrupts the workaround is unnecessary.
>>>>
>>>> Also, most recent maXTouch chips have a feature called RETRIGEN which, when
>>>> enabled, reasserts the interrupt line every cycle if there are messages
>>>> waiting. This also makes the workaround unnecessary.
>>>>
>>>> Note: the RETRIGEN feature is only in some firmware versions/chips, it's
>>>> not valid simply to enable the bit.
>>>
>>> Instead of trying to work around of misconfiguration for IRQ/firmware,
>>> can we simply error out of probe if we see a level interrupt with
>>> !RETRIGEN firmware?
>>>
>> I think for old firmwares, which doesn't support RETRIGEN feature, this
>> workaround is needed, otherwise we will break all old firmwares, which
>> configured with edge-triggered IRQ
>
> Do you know if there are any? I know Chrome OS firmware have RETRIGEN
> activated and they are pretty old (original Pixel is from 2013). But if
> we indeed have devices with edge interrupt and old not firmware that
> does not retrigger, I guess we'll have to keep it...
>
Honestly I don't know firmwares/chips which don't support RETRIGEN feature.
BUT Dyer originally authored this patch in 2012, I assume here "old"
firmware/chips means, those before 2012.
Thanks,
Jiada
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v5 10/17] net: sgi: ioc3-eth: rework skb rx handling
From: Jakub Kicinski @ 2019-08-21 20:37 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Jonathan Corbet, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Greg Kroah-Hartman,
Jiri Slaby, Evgeniy Polyakov, linux-doc, linux-kernel, linux-mips,
linux-input, netdev, linux-rtc, linux-serial,
Jesper Dangaard Brouer
In-Reply-To: <20190821162847.479c9967d4dc8026fe65fa0e@suse.de>
On Wed, 21 Aug 2019 16:28:47 +0200, Thomas Bogendoerfer wrote:
> > This looks like a DMA engine alignment requirement, more than an
> > optimization.
>
> that true, there are two constraints for the rx buffers, start must be aligned
> to 128 bytes and a buffer must not cross a 16kbyte boundary. I was already
> thinking of allocating pages and chop them up. Is there a Linux API available,
> which could help for implementing this ?
>
> I'll probably drop this patch or only change the skb_put stuff plus RX_BUF_SIZE
> define.
Sounds a little like frag allocator (napi_alloc_frag()/
netdev_alloc_frag()), but I'm not sure you'd have sufficient control
to skip over the 16k boundary.. Perhaps others have better suggestions.
^ permalink raw reply
* Re: [PATCH v1 03/63] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary
From: Dmitry Torokhov @ 2019-08-21 17:54 UTC (permalink / raw)
To: Jiada Wang; +Cc: nick, linux-input, linux-kernel, george_davis
In-Reply-To: <558e1227-7671-0838-d4e0-f234833c0973@mentor.com>
On Wed, Aug 21, 2019 at 10:26:31PM +0900, Jiada Wang wrote:
> Hi Dmitry
>
> On 2019/08/17 2:16, Dmitry Torokhov wrote:
> > On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
> > > From: Nick Dyer <nick.dyer@itdev.co.uk>
> > >
> > > The workaround of reading all messages until an invalid is received is a
> > > way of forcing the CHG line high, which means that when using
> > > edge-triggered interrupts the interrupt can be acquired.
> > >
> > > With level-triggered interrupts the workaround is unnecessary.
> > >
> > > Also, most recent maXTouch chips have a feature called RETRIGEN which, when
> > > enabled, reasserts the interrupt line every cycle if there are messages
> > > waiting. This also makes the workaround unnecessary.
> > >
> > > Note: the RETRIGEN feature is only in some firmware versions/chips, it's
> > > not valid simply to enable the bit.
> >
> > Instead of trying to work around of misconfiguration for IRQ/firmware,
> > can we simply error out of probe if we see a level interrupt with
> > !RETRIGEN firmware?
> >
> I think for old firmwares, which doesn't support RETRIGEN feature, this
> workaround is needed, otherwise we will break all old firmwares, which
> configured with edge-triggered IRQ
Do you know if there are any? I know Chrome OS firmware have RETRIGEN
activated and they are pretty old (original Pixel is from 2013). But if
we indeed have devices with edge interrupt and old not firmware that
does not retrigger, I guess we'll have to keep it...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS_ERR()
From: Dmitry Torokhov @ 2019-08-21 17:48 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: Pali Rohár, Enrico Weigelt, metux IT consult, linux-kernel,
linux-input, linux-ntfs-dev
In-Reply-To: <2cd7178e-9713-7678-a02d-dde91e990c1e@metux.net>
Hi,
On Wed, Aug 21, 2019 at 01:37:09PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 20.08.19 16:22, Pali Rohár wrote:
>
> Hi,
>
> > > In that case, wouldn't a comment be more suitable for that ?
> >
> > And why to add comment if current state of code is more-readable and
> > does not need it?
>
> Readability is probably a bit subjective :p
>
> With ongoing efforts of automatically identifying redundant code pathes,
> the current situation causes the same discussion coming up over and over
> again. Sooner or later somebody might get the idea to add a comment on
> that line, that it's exactly as intented :o
>
> OTOH, I'm unsure whether it's important to document that is particular
> error path is unlikely, while we don't do it in thousands of other
> places. IMHO, error pathes are supposed to be unlikely by nature,
> otherwise we wouldn't call it an error situation ;-)
>
> > People normally add comments to code which is problematic to understand
> > or is somehow tricky, no so obvious or document how should code behave.
>
> Yes, but isn't this case so obvious that it doesn't need any
> documentation at all ? Is it so important to never ever forget that this
> particular path is a rare situation ?
Because if I see "if (IS_ERR(...))" in an interrupt path I will try to
see if it can be optimized out, but in this particular case we document
it with explicit "unlikely" and I know that I do not need to bother.
The fact that there is unlikely in IS_ERR is an implementation detail.
It may be gone tomorrow. I do not want to have to remember all
implementation details of all kernel APIs and readjust the code all the
time as they are change underneath me.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 00/11] Face lift for bu21013_ts driver
From: Dmitry Torokhov @ 2019-08-21 17:42 UTC (permalink / raw)
To: Linus Walleij; +Cc: Linux Input, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdZo9so+5UoT3QpFmL_8NZT1d1i7Yab202RNn8gDfnPK7A@mail.gmail.com>
On Wed, Aug 21, 2019 at 02:39:41PM +0200, Linus Walleij wrote:
> On Sat, Aug 10, 2019 at 2:20 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>
> > So your patch has prompted me to take a look at the driver and
> > try to clean it up. I am sure I screwed up somewhere, but you said
> > you have the device, so please take a look at the series and
> > see if you can salvage them
>
> I will funnel patch 1/11 in the ARM SoC tree.
>
> The rest work fine except on the resource release in the error path. I had
> to do this:
>
> diff --git a/drivers/input/touchscreen/bu21013_ts.c
> b/drivers/input/touchscreen/bu21013_ts.c
> index c89a00a6e67c..bdae4cd4243a 100644
> --- a/drivers/input/touchscreen/bu21013_ts.c
> +++ b/drivers/input/touchscreen/bu21013_ts.c
> @@ -390,18 +390,18 @@ static int bu21013_init_chip(struct bu21013_ts *ts)
> return 0;
> }
>
> -static void bu21013_power_off(void *_ts)
> +static void bu21013_power_off(void *data)
> {
> - struct bu21013_ts *ts = ts;
> + struct regulator *regulator = data;
>
> - regulator_disable(ts->regulator);
> + regulator_disable(regulator);
> }
>
> -static void bu21013_disable_chip(void *_ts)
> +static void bu21013_disable_chip(void *data)
> {
> - struct bu21013_ts *ts = ts;
> + struct gpio_desc *gpiod = data;
>
> - gpiod_set_value(ts->cs_gpiod, 0);
> + gpiod_set_value(gpiod, 0);
> }
>
> static int bu21013_probe(struct i2c_client *client,
> @@ -488,7 +488,8 @@ static int bu21013_probe(struct i2c_client *client,
> return error;
> }
>
> - error = devm_add_action_or_reset(&client->dev, bu21013_power_off, ts);
> + error = devm_add_action_or_reset(&client->dev, bu21013_power_off,
> + ts->regulator);
> if (error) {
> dev_err(&client->dev, "failed to install power off handler\n");
> return error;
> @@ -505,7 +506,7 @@ static int bu21013_probe(struct i2c_client *client,
> gpiod_set_consumer_name(ts->cs_gpiod, "BU21013 CS");
>
> error = devm_add_action_or_reset(&client->dev,
> - bu21013_disable_chip, ts);
> + bu21013_disable_chip, ts->cs_gpiod);
> if (error) {
> dev_err(&client->dev,
> "failed to install chip disable handler\n");
>
>
> I think this is because when probe() fails it first free:s the devm_kzalloc()
> allocations, so the ts->foo will result in NULL dereference.
No, the release is done in opposite order of acquiring resources,
anything else would be madness and would not work.
The issue is this:
static void bu21013_disable_chip(void *_ts)
{
struct bu21013_ts *ts = ts;
which shuts up gcc about the fact that 'ts' is uninitialized, it should
have said "ts = _ts". I guess it is a lesson for me to not call the voi
d pointer argument almost the same name as the structure, as it is easy
to miss in the review. The compiler would not care in either case, but a
human might have noticed.
Can you please try making this change (and the same in power off
handler)?
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: hidraw: Fix invalid read in hidraw_ioctl
From: Alan Stern @ 2019-08-21 17:27 UTC (permalink / raw)
To: Jiri Kosina
Cc: andreyknvl, benjamin.tissoires, linux-input,
Kernel development list, USB list, syzkaller-bugs
In-Reply-To: <000000000000d45a4c0590a2d8bd@google.com>
The syzbot fuzzer has reported a pair of problems in the
hidraw_ioctl() function: slab-out-of-bounds read and use-after-free
read. An example of the first:
BUG: KASAN: slab-out-of-bounds in strlen+0x79/0x90 lib/string.c:525
Read of size 1 at addr ffff8881c8035f38 by task syz-executor.4/2833
CPU: 1 PID: 2833 Comm: syz-executor.4 Not tainted 5.3.0-rc2+ #1
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
print_address_description+0x6a/0x32c mm/kasan/report.c:351
__kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
kasan_report+0xe/0x12 mm/kasan/common.c:612
strlen+0x79/0x90 lib/string.c:525
strlen include/linux/string.h:281 [inline]
hidraw_ioctl+0x245/0xae0 drivers/hid/hidraw.c:446
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:509 [inline]
do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
__do_sys_ioctl fs/ioctl.c:720 [inline]
__se_sys_ioctl fs/ioctl.c:718 [inline]
__x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459829
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f7a68f6dc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000459829
RDX: 0000000000000000 RSI: 0000000080404805 RDI: 0000000000000004
RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f7a68f6e6d4
R13: 00000000004c21de R14: 00000000004d5620 R15: 00000000ffffffff
The two problems have the same cause: hidraw_ioctl() fails to test
whether the device has been removed. This patch adds the missing test.
Reported-and-tested-by: syzbot+5a6c4ec678a0c6ee84ba@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>
---
[as1910.hidraw-ioctl-fix]
drivers/hid/hidraw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: usb-devel/drivers/hid/hidraw.c
===================================================================
--- usb-devel.orig/drivers/hid/hidraw.c
+++ usb-devel/drivers/hid/hidraw.c
@@ -370,7 +370,7 @@ static long hidraw_ioctl(struct file *fi
mutex_lock(&minors_lock);
dev = hidraw_table[minor];
- if (!dev) {
+ if (!dev || !dev->exist) {
ret = -ENODEV;
goto out;
}
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in usbhid_close
From: Andrey Konovalov @ 2019-08-21 17:03 UTC (permalink / raw)
To: syzbot
Cc: Benjamin Tissoires, Jiri Kosina, linux-input, LKML, USB list,
syzkaller-bugs
In-Reply-To: <000000000000d6225c058f72a7df@google.com>
On Tue, Aug 6, 2019 at 3:18 PM syzbot
<syzbot+3268ee512f866a903602@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=117a9f42600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> dashboard link: https://syzkaller.appspot.com/bug?extid=3268ee512f866a903602
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+3268ee512f866a903602@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in __lock_acquire+0x302a/0x3b50
> kernel/locking/lockdep.c:3753
> Read of size 8 at addr ffff8881ceab68a0 by task syz-executor.0/3352
>
> CPU: 1 PID: 3352 Comm: syz-executor.0 Not tainted 5.3.0-rc2+ #25
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> print_address_description+0x6a/0x32c mm/kasan/report.c:351
> __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:612
> __lock_acquire+0x302a/0x3b50 kernel/locking/lockdep.c:3753
> lock_acquire+0x127/0x320 kernel/locking/lockdep.c:4412
> __raw_spin_lock_irq include/linux/spinlock_api_smp.h:128 [inline]
> _raw_spin_lock_irq+0x2d/0x40 kernel/locking/spinlock.c:167
> spin_lock_irq include/linux/spinlock.h:363 [inline]
> usbhid_close+0x51/0x210 drivers/hid/usbhid/hid-core.c:740
> hid_hw_close+0xa8/0xd0 drivers/hid/hid-core.c:2046
> drop_ref.part.0+0x32/0xe0 drivers/hid/hidraw.c:337
> drop_ref drivers/hid/hidraw.c:360 [inline]
> hidraw_release+0x34f/0x440 drivers/hid/hidraw.c:356
> __fput+0x2d7/0x840 fs/file_table.c:280
> task_work_run+0x13f/0x1c0 kernel/task_work.c:113
> exit_task_work include/linux/task_work.h:22 [inline]
> do_exit+0x8ef/0x2c50 kernel/exit.c:878
> do_group_exit+0x125/0x340 kernel/exit.c:982
> get_signal+0x466/0x23d0 kernel/signal.c:2728
> do_signal+0x88/0x14e0 arch/x86/kernel/signal.c:815
> exit_to_usermode_loop+0x1a2/0x200 arch/x86/entry/common.c:159
> prepare_exit_to_usermode arch/x86/entry/common.c:194 [inline]
> syscall_return_slowpath arch/x86/entry/common.c:274 [inline]
> do_syscall_64+0x45f/0x580 arch/x86/entry/common.c:299
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x459829
> Code: Bad RIP value.
> RSP: 002b:00007f123439dcf8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
> RAX: fffffffffffffe00 RBX: 000000000075bf28 RCX: 0000000000459829
> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 000000000075bf28
> RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000075bf2c
> R13: 00007ffe9281699f R14: 00007f123439e9c0 R15: 000000000075bf2c
>
> Allocated by task 0:
> (stack is not available)
>
> Freed by task 0:
> (stack is not available)
>
> The buggy address belongs to the object at ffff8881ceab6880
> which belongs to the cache shmem_inode_cache of size 1168
> The buggy address is located 32 bytes inside of
> 1168-byte region [ffff8881ceab6880, ffff8881ceab6d10)
> The buggy address belongs to the page:
> page:ffffea00073aad00 refcount:1 mapcount:0 mapping:ffff8881da115180
> index:0x0 compound_mapcount: 0
> flags: 0x200000000010200(slab|head)
> raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881da115180
> raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8881ceab6780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ffff8881ceab6800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff8881ceab6880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ^
> ffff8881ceab6900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8881ceab6980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
Looks like the same bug:
#syz dup: KASAN: slab-out-of-bounds Read in hidraw_ioctl
^ permalink raw reply
* Re: KASAN: use-after-free Read in usbhid_wait_io
From: Andrey Konovalov @ 2019-08-21 17:01 UTC (permalink / raw)
To: syzbot
Cc: Benjamin Tissoires, Jiri Kosina, linux-input, LKML, USB list,
syzkaller-bugs
In-Reply-To: <000000000000d56d4c058f5d6b2b@google.com>
On Mon, Aug 5, 2019 at 1:58 PM syzbot
<syzbot+cff772ea5b2812d504a9@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=136bed1a600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=792eb47789f57810
> dashboard link: https://syzkaller.appspot.com/bug?extid=cff772ea5b2812d504a9
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+cff772ea5b2812d504a9@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in test_bit
> include/asm-generic/bitops-instrumented.h:237 [inline]
> BUG: KASAN: use-after-free in usbhid_wait_io+0xc9/0x3a0
> drivers/hid/usbhid/hid-core.c:646
> Read of size 8 at addr ffff8881c84068c0 by task syz-executor.2/3548
>
> CPU: 1 PID: 3548 Comm: syz-executor.2 Not tainted 5.3.0-rc2+ #24
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> print_address_description+0x6a/0x32c mm/kasan/report.c:351
> __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:612
> check_memory_region_inline mm/kasan/generic.c:185 [inline]
> check_memory_region+0x128/0x190 mm/kasan/generic.c:192
> test_bit include/asm-generic/bitops-instrumented.h:237 [inline]
> usbhid_wait_io+0xc9/0x3a0 drivers/hid/usbhid/hid-core.c:646
> usbhid_init_reports+0x119/0x320 drivers/hid/usbhid/hid-core.c:774
> hiddev_ioctl+0x10ed/0x1550 drivers/hid/usbhid/hiddev.c:792
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:509 [inline]
> do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
> ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
> __do_sys_ioctl fs/ioctl.c:720 [inline]
> __se_sys_ioctl fs/ioctl.c:718 [inline]
> __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x459829
> Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fb23788ec78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000459829
> RDX: 0000000020005700 RSI: 0000000040184810 RDI: 0000000000000008
> RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb23788f6d4
> R13: 00000000004c2104 R14: 00000000004d5490 R15: 00000000ffffffff
>
> Allocated by task 12:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_kmalloc mm/kasan/common.c:487 [inline]
> __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460
> slab_post_alloc_hook mm/slab.h:520 [inline]
> slab_alloc_node mm/slub.c:2766 [inline]
> __kmalloc_node_track_caller+0xd0/0x230 mm/slub.c:4361
> __kmalloc_reserve.isra.0+0x39/0xe0 net/core/skbuff.c:141
> __alloc_skb+0xef/0x5a0 net/core/skbuff.c:209
> alloc_skb include/linux/skbuff.h:1055 [inline]
> alloc_uevent_skb+0x7b/0x210 lib/kobject_uevent.c:289
> uevent_net_broadcast_untagged lib/kobject_uevent.c:325 [inline]
> kobject_uevent_net_broadcast lib/kobject_uevent.c:408 [inline]
> kobject_uevent_env+0x8ee/0x1160 lib/kobject_uevent.c:592
> __device_release_driver drivers/base/dd.c:1140 [inline]
> device_release_driver_internal+0x3c4/0x4c0 drivers/base/dd.c:1151
> bus_remove_device+0x2dc/0x4a0 drivers/base/bus.c:556
> device_del+0x420/0xb10 drivers/base/core.c:2288
> usb_disconnect+0x4c3/0x8d0 drivers/usb/core/hub.c:2225
> hub_port_connect drivers/usb/core/hub.c:4949 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> Freed by task 240:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_slab_free+0x130/0x180 mm/kasan/common.c:449
> slab_free_hook mm/slub.c:1423 [inline]
> slab_free_freelist_hook mm/slub.c:1470 [inline]
> slab_free mm/slub.c:3012 [inline]
> kfree+0xe4/0x2f0 mm/slub.c:3953
> skb_free_head+0x8b/0xa0 net/core/skbuff.c:591
> skb_release_data+0x41f/0x7c0 net/core/skbuff.c:611
> skb_release_all+0x46/0x60 net/core/skbuff.c:665
> __kfree_skb net/core/skbuff.c:679 [inline]
> consume_skb net/core/skbuff.c:838 [inline]
> consume_skb+0xd9/0x320 net/core/skbuff.c:832
> skb_free_datagram+0x16/0xf0 net/core/datagram.c:328
> netlink_recvmsg+0x65e/0xee0 net/netlink/af_netlink.c:1996
> sock_recvmsg_nosec net/socket.c:871 [inline]
> sock_recvmsg net/socket.c:889 [inline]
> sock_recvmsg+0xca/0x110 net/socket.c:885
> ___sys_recvmsg+0x271/0x5a0 net/socket.c:2480
> __sys_recvmsg+0xe9/0x1b0 net/socket.c:2537
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> The buggy address belongs to the object at ffff8881c8406880
> which belongs to the cache kmalloc-1k of size 1024
> The buggy address is located 64 bytes inside of
> 1024-byte region [ffff8881c8406880, ffff8881c8406c80)
> The buggy address belongs to the page:
> page:ffffea0007210100 refcount:1 mapcount:0 mapping:ffff8881da002280
> index:0x0 compound_mapcount: 0
> flags: 0x200000000010200(slab|head)
> raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881da002280
> raw: 0000000000000000 00000000000e000e 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8881c8406780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881c8406800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff8881c8406880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff8881c8406900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881c8406980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
Looks like the same bug:
#syz dup: general protection fault in __pm_runtime_resume
^ permalink raw reply
* Re: KASAN: use-after-free Read in hiddev_ioctl
From: Andrey Konovalov @ 2019-08-21 17:01 UTC (permalink / raw)
To: syzbot
Cc: Benjamin Tissoires, Jiri Kosina, linux-input, LKML, USB list,
syzkaller-bugs
In-Reply-To: <000000000000da58ed058f72a7f1@google.com>
On Tue, Aug 6, 2019 at 3:18 PM syzbot
<syzbot+5e9ed50a49eb77802d0e@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1732258a600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> dashboard link: https://syzkaller.appspot.com/bug?extid=5e9ed50a49eb77802d0e
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5e9ed50a49eb77802d0e@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in __mutex_lock_common
> kernel/locking/mutex.c:912 [inline]
> BUG: KASAN: use-after-free in __mutex_lock+0xf23/0x1360
> kernel/locking/mutex.c:1077
> Read of size 8 at addr ffff8881cf955468 by task syz-executor.1/19529
>
> CPU: 0 PID: 19529 Comm: syz-executor.1 Not tainted 5.3.0-rc2+ #25
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> print_address_description+0x6a/0x32c mm/kasan/report.c:351
> __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> kasan_report+0xe/0x12 mm/kasan/common.c:612
> __mutex_lock_common kernel/locking/mutex.c:912 [inline]
> __mutex_lock+0xf23/0x1360 kernel/locking/mutex.c:1077
> hiddev_ioctl+0xea/0x1550 drivers/hid/usbhid/hiddev.c:607
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:509 [inline]
> do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696
> ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713
> __do_sys_ioctl fs/ioctl.c:720 [inline]
> __se_sys_ioctl fs/ioctl.c:718 [inline]
> __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
> do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x459829
> Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f0ec5dd7c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000459829
> RDX: 0000000020000380 RSI: 0000000081044804 RDI: 0000000000000005
> RBP: 000000000075c070 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f0ec5dd86d4
> R13: 00000000004c2249 R14: 00000000004d55f8 R15: 00000000ffffffff
>
> Allocated by task 2777:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_kmalloc mm/kasan/common.c:487 [inline]
> __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460
> kmalloc include/linux/slab.h:552 [inline]
> kzalloc include/linux/slab.h:748 [inline]
> hiddev_connect+0x242/0x5b0 drivers/hid/usbhid/hiddev.c:900
> hid_connect+0x239/0xbb0 drivers/hid/hid-core.c:1882
> hid_hw_start drivers/hid/hid-core.c:1981 [inline]
> hid_hw_start+0xa2/0x130 drivers/hid/hid-core.c:1972
> appleir_probe+0x13e/0x1a0 drivers/hid/hid-appleir.c:308
> hid_device_probe+0x2be/0x3f0 drivers/hid/hid-core.c:2209
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> hid_add_device+0x33c/0x990 drivers/hid/hid-core.c:2365
> usbhid_probe+0xa81/0xfa0 drivers/hid/usbhid/hid-core.c:1386
> usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> process_scheduled_works kernel/workqueue.c:2331 [inline]
> worker_thread+0x7ab/0xe20 kernel/workqueue.c:2417
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> Freed by task 2777:
> save_stack+0x1b/0x80 mm/kasan/common.c:69
> set_track mm/kasan/common.c:77 [inline]
> __kasan_slab_free+0x130/0x180 mm/kasan/common.c:449
> slab_free_hook mm/slub.c:1423 [inline]
> slab_free_freelist_hook mm/slub.c:1470 [inline]
> slab_free mm/slub.c:3012 [inline]
> kfree+0xe4/0x2f0 mm/slub.c:3953
> hiddev_connect.cold+0x45/0x5c drivers/hid/usbhid/hiddev.c:914
> hid_connect+0x239/0xbb0 drivers/hid/hid-core.c:1882
> hid_hw_start drivers/hid/hid-core.c:1981 [inline]
> hid_hw_start+0xa2/0x130 drivers/hid/hid-core.c:1972
> appleir_probe+0x13e/0x1a0 drivers/hid/hid-appleir.c:308
> hid_device_probe+0x2be/0x3f0 drivers/hid/hid-core.c:2209
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> hid_add_device+0x33c/0x990 drivers/hid/hid-core.c:2365
> usbhid_probe+0xa81/0xfa0 drivers/hid/usbhid/hid-core.c:1386
> usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> really_probe+0x281/0x650 drivers/base/dd.c:548
> driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> __device_attach+0x217/0x360 drivers/base/dd.c:882
> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> device_add+0xae6/0x16f0 drivers/base/core.c:2114
> usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> process_scheduled_works kernel/workqueue.c:2331 [inline]
> worker_thread+0x7ab/0xe20 kernel/workqueue.c:2417
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
>
> The buggy address belongs to the object at ffff8881cf955400
> which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 104 bytes inside of
> 512-byte region [ffff8881cf955400, ffff8881cf955600)
> The buggy address belongs to the page:
> page:ffffea00073e5500 refcount:1 mapcount:0 mapping:ffff8881da002500
> index:0x0 compound_mapcount: 0
> flags: 0x200000000010200(slab|head)
> raw: 0200000000010200 ffffea0007659680 0000000600000003 ffff8881da002500
> raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8881cf955300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881cf955380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff8881cf955400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff8881cf955480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8881cf955500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
Looks like the same bug:
#syz dup: general protection fault in __pm_runtime_resume
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in hidraw_ioctl
From: Andrey Konovalov @ 2019-08-21 16:50 UTC (permalink / raw)
To: Alan Stern
Cc: syzbot, Benjamin Tissoires, Jiri Kosina, linux-input, LKML,
USB list, syzkaller-bugs
In-Reply-To: <CAAeHK+z-o9naQXZoxwTXRh2WWQzFiRU9XruabNTTm31_1AbjAw@mail.gmail.com>
On Wed, Aug 21, 2019 at 6:26 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Wed, Aug 21, 2019 at 6:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, 21 Aug 2019, Andrey Konovalov wrote:
> >
> > > On Wed, Aug 21, 2019 at 3:37 PM syzbot
> > > <syzbot+5a6c4ec678a0c6ee84ba@syzkaller.appspotmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot has tested the proposed patch but the reproducer still triggered
> > > > crash:
> > > > KASAN: slab-out-of-bounds Read in hidraw_ioctl
> > >
> > > Same here, a different bug.
> >
> > It looks like I've got the fix for both these bugs. Testing now...
>
> Great! Do you think "BUG: bad usercopy in hidraw_ioctl" can also be
> fixed by one of those fixes?
We actually have a bunch of other non reproducible bug reports that
come from HID. I think I'll dup them into these two bugs that you've
fixed, and we'll see if syzkaller triggers them again once the fixes
are upstream.
>
> >
> > > > Tested on:
> > > >
> > > > commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > > > git tree: https://github.com/google/kasan.git
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14f14a1e600000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > patch: https://syzkaller.appspot.com/x/patch.diff?x=171cd95a600000
> >
> > Why don't these patch-test reports include the dashboard link? It sure
> > would be handy to have a copy of it here.
Sorry, didn't notice this comment. This should be easy to implement,
I'll look into that, thanks!
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in hidraw_ioctl
From: Andrey Konovalov @ 2019-08-21 16:26 UTC (permalink / raw)
To: Alan Stern
Cc: syzbot, Benjamin Tissoires, Jiri Kosina, linux-input, LKML,
USB list, syzkaller-bugs
In-Reply-To: <Pine.LNX.4.44L0.1908211223070.1816-100000@iolanthe.rowland.org>
On Wed, Aug 21, 2019 at 6:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 21 Aug 2019, Andrey Konovalov wrote:
>
> > On Wed, Aug 21, 2019 at 3:37 PM syzbot
> > <syzbot+5a6c4ec678a0c6ee84ba@syzkaller.appspotmail.com> wrote:
> > >
> > > Hello,
> > >
> > > syzbot has tested the proposed patch but the reproducer still triggered
> > > crash:
> > > KASAN: slab-out-of-bounds Read in hidraw_ioctl
> >
> > Same here, a different bug.
>
> It looks like I've got the fix for both these bugs. Testing now...
Great! Do you think "BUG: bad usercopy in hidraw_ioctl" can also be
fixed by one of those fixes?
>
> > > Tested on:
> > >
> > > commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree: https://github.com/google/kasan.git
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=14f14a1e600000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > patch: https://syzkaller.appspot.com/x/patch.diff?x=171cd95a600000
>
> Why don't these patch-test reports include the dashboard link? It sure
> would be handy to have a copy of it here.
>
> Alan Stern
>
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in hidraw_ioctl
From: Alan Stern @ 2019-08-21 16:24 UTC (permalink / raw)
To: Andrey Konovalov
Cc: syzbot, Benjamin Tissoires, Jiri Kosina, linux-input, LKML,
USB list, syzkaller-bugs
In-Reply-To: <CAAeHK+xQc5Ce6TwtERTmQ+6qSbuAmGikxCU5SNTdcDAynDEiig@mail.gmail.com>
On Wed, 21 Aug 2019, Andrey Konovalov wrote:
> On Wed, Aug 21, 2019 at 3:37 PM syzbot
> <syzbot+5a6c4ec678a0c6ee84ba@syzkaller.appspotmail.com> wrote:
> >
> > Hello,
> >
> > syzbot has tested the proposed patch but the reproducer still triggered
> > crash:
> > KASAN: slab-out-of-bounds Read in hidraw_ioctl
>
> Same here, a different bug.
It looks like I've got the fix for both these bugs. Testing now...
> > Tested on:
> >
> > commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > git tree: https://github.com/google/kasan.git
> > console output: https://syzkaller.appspot.com/x/log.txt?x=14f14a1e600000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > patch: https://syzkaller.appspot.com/x/patch.diff?x=171cd95a600000
Why don't these patch-test reports include the dashboard link? It sure
would be handy to have a copy of it here.
Alan Stern
^ permalink raw reply
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