Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210122001326.14263-1-sonnysasaka@chromium.org>
 <20210122001326.14263-2-sonnysasaka@chromium.org> <CABBYNZJs_rzBSHOjvqg5F4u6gxX7LcNUc77KtzHEr5L7Y5Zd-A@mail.gmail.com>
 <CAO271mkMtmDm_MayCX-+Dddg3aL1uv=GD3egCGJ6gDCEHowp8g@mail.gmail.com> <CAO271mkhpd8KOA=a3E66VpJUj01PONjmpS=m+U9MtwSWnUQ6BA@mail.gmail.com>
In-Reply-To: <CAO271mkhpd8KOA=a3E66VpJUj01PONjmpS=m+U9MtwSWnUQ6BA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 25 Jan 2021 13:50:54 -0800
Message-ID: <CABBYNZ+GazVResDzTtP4Oyt+duG4iNnPEC7CDu8JB5iPsrfzCw@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 2/2] input/hog: Do not create UHID if report map
 is broken
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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