diff mbox series

[BlueZ,v2,2/2] input/hog: Do not create UHID if report map is broken

Message ID 20210122001326.14263-2-sonnysasaka@chromium.org
State New
Headers show
Series None | expand

Commit Message

Sonny Sasaka Jan. 22, 2021, 12:13 a.m. UTC 
The report map in the cache could be dirty, for example when reading a
report map from peer was cancelled, we should be able to detect it and
not try to create UHID. Instead we will read it again from the peer.

---
 profiles/input/hog-lib.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Sonny Sasaka Jan. 25, 2021, 7:35 p.m. UTC | #1 
Hi Luiz,

I have been trying to reproduce the issue again but it turns out to be
very rare. Let's defer this patch until I can get a clear log of what
is happening and why we get the corrupted cache.



On Thu, Jan 21, 2021 at 5:24 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>

> Hi Luiz,

>

> On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz

> <luiz.dentz@gmail.com> wrote:

> >

> > Hi Sonny,

> >

> > On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> > >

> > > The report map in the cache could be dirty, for example when reading a

> > > report map from peer was cancelled, we should be able to detect it and

> > > not try to create UHID. Instead we will read it again from the peer.

> >

> > Don't we clean the cache if it had failed? Or you meant to say the

> > read long procedure was not complete so we got just part of the report

> > map?

> Looks like this is the case. It happened to me once when I cancel

> reconnection (trigger pairing mode during reconnection) from the

> keyboard side. It's hard to confirm since I have to get the timing

> right.

>

> > In that case we should have failed

> I agree. However it seems that the code already tries to fail by

> looking at the status inside report_map_read_cb, but somehow it still

> gets through. It could be the keyboard bug that we have to detect

> anyway?

>

> > also if we need to protect

> > uhid from malformed report map, which sounds like a kernel bug, then

> > we should at least have it inside bt_uhid instance so we can at least

> > attempt to have some unit testing done with broken report maps.

> >

> > > ---

> > >  profiles/input/hog-lib.c | 21 ++++++++++++++++++---

> > >  1 file changed, 18 insertions(+), 3 deletions(-)

> > >

> > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c

> > > index 089f42826..d6a3bda4d 100644

> > > --- a/profiles/input/hog-lib.c

> > > +++ b/profiles/input/hog-lib.c

> > > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > >         struct uhid_event ev;

> > >         ssize_t vlen = report_map_len;

> > >         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */

> > > -       int i, err;

> > > +       int i, err, collection_depth = 0;

> > >         GError *gerr = NULL;

> > >

> > >         DBG("Report MAP:");

> > > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > >                         if (!long_item && (value[i] & 0xfc) == 0x84)

> > >                                 hog->has_report_id = TRUE;

> > >

> > > +                       // Start Collection

> > > +                       if (value[i] == 0xa1)

> > > +                               collection_depth++;

> > > +

> > > +                       // End Collection

> > > +                       if (value[i] == 0xc0)

> > > +                               collection_depth--;

> > > +

> > >                         DBG("\t%s", item2string(itemstr, &value[i], ilen));

> > >

> > >                         i += ilen;

> > > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > >

> > >                         /* Just print remaining items at once and break */

> > >                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));

> > > -                       break;

> > > +                       return;

> > >                 }

> > >         }

> > >

> > > +       if (collection_depth != 0) {

> > > +               error("Report Map error: unbalanced collection");

> > > +               return;

> > > +       }

> > > +

> > >         /* create uHID device */

> > >         memset(&ev, 0, sizeof(ev));

> > >         ev.type = UHID_CREATE;

> > > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)

> > >                          * UHID to optimize reconnection.

> > >                          */

> > >                         uhid_create(hog, report_map.value, report_map.length);

> > > -               } else {

> > > +               }

> > > +

> > > +               if (!hog->uhid_created) {

> > >                         read_char(hog, hog->attrib, value_handle,

> > >                                                 report_map_read_cb, hog);

> > >                 }

> > > --

> > > 2.29.2

> > >

> >

> >

> > --

> > Luiz Augusto von Dentz
Luiz Augusto von Dentz Jan. 25, 2021, 9:50 p.m. UTC | #2 
Hi Sonny,

On Mon, Jan 25, 2021 at 11:36 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>

> Hi Luiz,

>

> I have been trying to reproduce the issue again but it turns out to be

> very rare. Let's defer this patch until I can get a clear log of what

> is happening and why we get the corrupted cache.


Ok, let me update in the pw, if you see this again let me know.

>

>

> On Thu, Jan 21, 2021 at 5:24 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> >

> > Hi Luiz,

> >

> > On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz

> > <luiz.dentz@gmail.com> wrote:

> > >

> > > Hi Sonny,

> > >

> > > On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:

> > > >

> > > > The report map in the cache could be dirty, for example when reading a

> > > > report map from peer was cancelled, we should be able to detect it and

> > > > not try to create UHID. Instead we will read it again from the peer.

> > >

> > > Don't we clean the cache if it had failed? Or you meant to say the

> > > read long procedure was not complete so we got just part of the report

> > > map?

> > Looks like this is the case. It happened to me once when I cancel

> > reconnection (trigger pairing mode during reconnection) from the

> > keyboard side. It's hard to confirm since I have to get the timing

> > right.

> >

> > > In that case we should have failed

> > I agree. However it seems that the code already tries to fail by

> > looking at the status inside report_map_read_cb, but somehow it still

> > gets through. It could be the keyboard bug that we have to detect

> > anyway?

> >

> > > also if we need to protect

> > > uhid from malformed report map, which sounds like a kernel bug, then

> > > we should at least have it inside bt_uhid instance so we can at least

> > > attempt to have some unit testing done with broken report maps.

> > >

> > > > ---

> > > >  profiles/input/hog-lib.c | 21 ++++++++++++++++++---

> > > >  1 file changed, 18 insertions(+), 3 deletions(-)

> > > >

> > > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c

> > > > index 089f42826..d6a3bda4d 100644

> > > > --- a/profiles/input/hog-lib.c

> > > > +++ b/profiles/input/hog-lib.c

> > > > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > > >         struct uhid_event ev;

> > > >         ssize_t vlen = report_map_len;

> > > >         char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */

> > > > -       int i, err;

> > > > +       int i, err, collection_depth = 0;

> > > >         GError *gerr = NULL;

> > > >

> > > >         DBG("Report MAP:");

> > > > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > > >                         if (!long_item && (value[i] & 0xfc) == 0x84)

> > > >                                 hog->has_report_id = TRUE;

> > > >

> > > > +                       // Start Collection

> > > > +                       if (value[i] == 0xa1)

> > > > +                               collection_depth++;

> > > > +

> > > > +                       // End Collection

> > > > +                       if (value[i] == 0xc0)

> > > > +                               collection_depth--;

> > > > +

> > > >                         DBG("\t%s", item2string(itemstr, &value[i], ilen));

> > > >

> > > >                         i += ilen;

> > > > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,

> > > >

> > > >                         /* Just print remaining items at once and break */

> > > >                         DBG("\t%s", item2string(itemstr, &value[i], vlen - i));

> > > > -                       break;

> > > > +                       return;

> > > >                 }

> > > >         }

> > > >

> > > > +       if (collection_depth != 0) {

> > > > +               error("Report Map error: unbalanced collection");

> > > > +               return;

> > > > +       }

> > > > +

> > > >         /* create uHID device */

> > > >         memset(&ev, 0, sizeof(ev));

> > > >         ev.type = UHID_CREATE;

> > > > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)

> > > >                          * UHID to optimize reconnection.

> > > >                          */

> > > >                         uhid_create(hog, report_map.value, report_map.length);

> > > > -               } else {

> > > > +               }

> > > > +

> > > > +               if (!hog->uhid_created) {

> > > >                         read_char(hog, hog->attrib, value_handle,

> > > >                                                 report_map_read_cb, hog);

> > > >                 }

> > > > --

> > > > 2.29.2

> > > >

> > >

> > >

> > > --

> > > Luiz Augusto von Dentz




-- 
Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 089f42826..d6a3bda4d 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -946,7 +946,7 @@  static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 	struct uhid_event ev;
 	ssize_t vlen = report_map_len;
 	char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
-	int i, err;
+	int i, err, collection_depth = 0;
 	GError *gerr = NULL;
 
 	DBG("Report MAP:");
@@ -960,6 +960,14 @@  static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 			if (!long_item && (value[i] & 0xfc) == 0x84)
 				hog->has_report_id = TRUE;
 
+			// Start Collection
+			if (value[i] == 0xa1)
+				collection_depth++;
+
+			// End Collection
+			if (value[i] == 0xc0)
+				collection_depth--;
+
 			DBG("\t%s", item2string(itemstr, &value[i], ilen));
 
 			i += ilen;
@@ -968,10 +976,15 @@  static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
 
 			/* Just print remaining items at once and break */
 			DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
-			break;
+			return;
 		}
 	}
 
+	if (collection_depth != 0) {
+		error("Report Map error: unbalanced collection");
+		return;
+	}
+
 	/* create uHID device */
 	memset(&ev, 0, sizeof(ev));
 	ev.type = UHID_CREATE;
@@ -1365,7 +1378,9 @@  static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
 			 * UHID to optimize reconnection.
 			 */
 			uhid_create(hog, report_map.value, report_map.length);
-		} else {
+		}
+
+		if (!hog->uhid_created) {
 			read_char(hog, hog->attrib, value_handle,
 						report_map_read_cb, hog);
 		}