On Thu, Oct 28, 2021 at 08:40:03PM +0200, Anton Lindqvist wrote:
> On Thu, Oct 28, 2021 at 08:08:36PM +0200, Damien Couderc wrote:
> > Le 26/10/2021 à 19:03, Anton Lindqvist a écrit :
> > > On Tue, Oct 26, 2021 at 05:58:12PM +0200, Damien Couderc wrote:
> > > > Le 26/10/2021 à 16:11, Anton Lindqvist a écrit :
> > > > > On Tue, Oct 26, 2021 at 09:50:41AM +0200, Damien Couderc wrote:
> > > > > > Le 24/10/2021 à 21:45, Anton Lindqvist a écrit :
> > > > > > > On Sun, Oct 24, 2021 at 03:03:01PM +0200, Damien Couderc wrote:
> > > > > > > > Hi,
> > > > > > > > I got a page fault with upd(4) since 7.0.
> > > > > > > >
> > > > > > > > The problem began with the last revision of upd.c (1.30):
> > > > > > > >
> > > > > > > > ===================================================================
> > > > > > > > RCS file: /cvs/src/sys/dev/usb/upd.c,v
> > > > > > > > retrieving revision 1.29
> > > > > > > > retrieving revision 1.30
> > > > > > > > diff -u -r1.29 -r1.30
> > > > > > > > --- src/sys/dev/usb/upd.c 2021/03/08 14:35:57 1.29
> > > > > > > > +++ src/sys/dev/usb/upd.c 2021/08/06 17:46:45 1.30
> > > > > > > > @@ -1,4 +1,4 @@
> > > > > > > > -/* $OpenBSD: upd.c,v 1.29 2021/03/08 14:35:57 jcs Exp $ */
> > > > > > > > +/* $OpenBSD: upd.c,v 1.30 2021/08/06 17:46:45 abieber Exp
> > > > > > > > $ */
> > > > > > > >
> > > > > > > > /*
> > > > > > > > * Copyright (c) 2015 David Higgs <hig...@gmail.com>
> > > > > > > > @@ -167,7 +167,7 @@
> > > > > > > > if (upd_lookup_usage_entry(desc, size,
> > > > > > > > upd_usage_roots + i, &item)) {
> > > > > > > > ret = UMATCH_VENDOR_PRODUCT;
> > > > > > > > - break;
> > > > > > > > + uha->claimed[item.report_ID] = 1;
> > > > > > > > }
> > > > > > > >
> > > > > > > > return (ret);
> > > > > > > >
> > > > > > > > ===================================================================
> > > > > > > >
> > > > > > > > The uha.claimed array is allocated using uha.nreports as its
> > > > > > > > size while
> > > > > > > > upd_match() is looping through the number of items of
> > > > > > > > upd_usage_roots.
> > > > > > > >
> > > > > > > > In my case uha.nreports is equal to zero so uha.claimed is
> > > > > > > > null, hence
> > > > > > > > setting uha->claimed[n] is failing.
> > > > > > > >
> > > > > > > > As I'm not familiar with the HID code I did not yet understood
> > > > > > > > the
> > > > > > > > relation between upd_usage_roots and the claimed array but as
> > > > > > > > we're
> > > > > > > > talking about UPS attached computers I though the issue would
> > > > > > > > sensible
> > > > > > > > enough to make a quick reporting.
> > > > > > > >
> > > > > > > > You'll find a dmesg with options UPD_DEBUG and UHIDEV_DEBUG set
> > > > > > > > and the
> > > > > > > > following patch applied to circumvent the page fault and
> > > > > > > > provide some debug:
> > > > > > > Could you try the following diff, looks like an unsigned wrap
> > > > > > > around.
> > > > > > >
> > > > > > > Index: dev/usb/uhidev.h
> > > > > > > ===================================================================
> > > > > > > RCS file: /cvs/src/sys/dev/usb/uhidev.h,v
> > > > > > > retrieving revision 1.32
> > > > > > > diff -u -p -r1.32 uhidev.h
> > > > > > > --- dev/usb/uhidev.h 12 Sep 2021 06:58:08 -0000 1.32
> > > > > > > +++ dev/usb/uhidev.h 24 Oct 2021 19:44:52 -0000
> > > > > > > @@ -82,7 +82,7 @@ struct uhidev_attach_arg {
> > > > > > > struct uhidev_softc *parent;
> > > > > > > uint8_t reportid;
> > > > > > > #define UHIDEV_CLAIM_MULTIPLE_REPORTID 255
> > > > > > > - uint8_t nreports;
> > > > > > > + u_int nreports;
> > > > > > > uint8_t *claimed;
> > > > > > > };
> > > > > > >
> > > > > > Hello Anton,
> > > > > >
> > > > > > I made a quick test and nreports is now set with 256 but I still
> > > > > > get the
> > > > > > page fault.
> > > > > >
> > > > > > I'll check the details ASAP.
> > > > >
> > > > > Do you have a backtrace?
> > > > >
> > > >
> > > > I only have screenshots of ddb.
> > > >
> > > > With the default kernel :
> > > > http://cromagnon.petrocore.eu/ss_upd_ddb_default.jpg
> > > >
> > > > With a custom kernel for a bit more data :
> > > > http://cromagnon.petrocore.eu/ss_upd_ddb_custom.jpg
> > >
> > > Looks like a NULL deref. My guess would be item.report_ID going out of
> > > bounds on uha->claimed. Inspecting item.report_ID could provide some
> > > hints. This in turn could be caused by uhidev_maxrepid() looking for
> > > hid_none items whereas upd_lookup_usage_entry() uses hid_feature.
> > >
> > > Also, it would be interesting to see the descriptor of this device. If
> > > you make upd_match() unconditionally return 0 it should hopefully^W
> > > attach as uhid device; making it possible to extract the report using
> > > usbhidctl replacing X with the attached device:
> > >
> > > # usbhidctl -R -f /dev/uhidX
> > >
> >
> > Hi !
> >
> > I set uhidevdebug to enable more debug messages and it gets more
> > interesting as we can see that we enter twice in upd_match() but the
> > second time uha.claimed is not allocated.
> >
> > I'll dig for more ASAP.
> >
> > Which uhid device would you want to get the report for ?
> >
> > Below the diff of the changes I made yet and the corresponding dmesg.
>
> Oh, I think UHIDEV_CLAIM_MULTIPLE_REPORTID is conflicting with an actual
> report ID.
Sorry, this is the diff I intended to send.
Index: dev/usb/uhidev.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/uhidev.h,v
retrieving revision 1.32
diff -u -p -r1.32 uhidev.h
--- dev/usb/uhidev.h 12 Sep 2021 06:58:08 -0000 1.32
+++ dev/usb/uhidev.h 28 Oct 2021 19:07:59 -0000
@@ -80,9 +80,9 @@ struct uhidev {
struct uhidev_attach_arg {
struct usb_attach_arg *uaa;
struct uhidev_softc *parent;
- uint8_t reportid;
-#define UHIDEV_CLAIM_MULTIPLE_REPORTID 255
- uint8_t nreports;
+ u_int reportid;
+#define UHIDEV_CLAIM_MULTIPLE_REPORTID 0xffffffff
+ u_int nreports;
uint8_t *claimed;
};
