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>
In-Reply-To: <20210122001326.14263-2-sonnysasaka@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 Jan 2021 16:36:55 -0800
Message-ID: <CABBYNZJs_rzBSHOjvqg5F4u6gxX7LcNUc77KtzHEr5L7Y5Zd-A@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 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? In that case we should have failed, 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