DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9ACB92081F
MIME-Version: 1.0
References: <20181031081508.25927-1-acho@suse.com> <20181031081508.25927-2-acho@suse.com>
In-Reply-To: <20181031081508.25927-2-acho@suse.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 2 Nov 2018 12:54:27 +0200
Message-ID: <CABBYNZKNiqLDAnV0hB0tZ_=fCr6RK8Q7Wyn3UTWk=PXSREMupg@mail.gmail.com>
Subject: Re: [PATCH 1/3 BlueZ] hcidump: fixed AMP Assoc dump heap-over-flow
To:     "Cho, Yu-Chen" <acho@suse.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        jlee@suse.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi,
On Wed, Oct 31, 2018 at 11:08 AM Cho, Yu-Chen <acho@suse.com> wrote:
>
> amp_assoc_dump() didn't check the length of amp assoc struct of
> Type-Length-Value (TLV) triplets, and the Connected Chan List
> (number of triplets) is also need to check, or there are wrong
> length for the number of triplets.
>
> Signed-off-by: Cho, Yu-Chen <acho@suse.com>

Please remove the Signed-off-by, we don't use that in userspace.

> ---
>  tools/parser/amp.c | 65 ++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/tools/parser/amp.c b/tools/parser/amp.c
> index 158ca4a75..bd7f91000 100644
> --- a/tools/parser/amp.c
> +++ b/tools/parser/amp.c
> @@ -33,7 +33,7 @@ static void amp_dump_chanlist(int level, struct amp_tlv *tlv, char *prefix)
>         struct amp_country_triplet *triplet;
>         int i, num;
>
> -       num = (tlv->len - sizeof(*chan_list)) / sizeof(*triplet);
> +       num = sizeof(*chan_list->triplets) / sizeof(*chan_list->triplets[0]);
>
>         printf("%s (number of triplets %d)\n", prefix, num);
>
> @@ -80,47 +80,50 @@ void amp_assoc_dump(int level, uint8_t *assoc, uint16_t len)
>
>                 p_indent(level+1, 0);
>
> -               switch (tlv->type) {
> -               case A2MP_MAC_ADDR_TYPE:
> -                       if (tlvlen != 6)
> -                               break;
> -                       printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
> +               if (tlvlen !=0) {
> +                       switch (tlv->type) {
> +                       case A2MP_MAC_ADDR_TYPE:
> +                               if (tlvlen != 6)
> +                                       break;
> +                               printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
>                                         tlv->val[0], tlv->val[1], tlv->val[2],
>                                         tlv->val[3], tlv->val[4], tlv->val[5]);
> -                       break;
> -
> -               case A2MP_PREF_CHANLIST_TYPE:
> -                       amp_dump_chanlist(level, tlv, "Preferred Chan List");
> -                       break;
> +                               break;
>
> -               case A2MP_CONNECTED_CHAN:
> -                       amp_dump_chanlist(level, tlv, "Connected Chan List");
> -                       break;
> +                       case A2MP_PREF_CHANLIST_TYPE:
> +                               amp_dump_chanlist(level, tlv, "Preferred Chan List");
> +                               break;
>
> -               case A2MP_PAL_CAP_TYPE:
> -                       if (tlvlen != 4)
> +                       case A2MP_CONNECTED_CHAN:
> +                               amp_dump_chanlist(level, tlv, "Connected Chan List");
>                                 break;
> -                       printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
> +
> +                       case A2MP_PAL_CAP_TYPE:
> +                               if (tlvlen != 4)
> +                                       break;
> +                               printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
>                                         tlv->val[0], tlv->val[1], tlv->val[2],
>                                         tlv->val[3]);
> -                       break;
> -
> -               case A2MP_PAL_VER_INFO:
> -                       if (tlvlen != 5)
>                                 break;
> -                       ver = (struct amp_pal_ver *) tlv->val;
> -                       printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
> +
> +                       case A2MP_PAL_VER_INFO:
> +                               if (tlvlen != 5)
> +                                       break;
> +                               ver = (struct amp_pal_ver *) tlv->val;
> +                               printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
>                                         ver->ver, btohs(ver->company_id),
>                                         btohs(ver->sub_ver));
> -                       break;
> +                               break;
>
> -               default:
> -                       printf("Unrecognized type %d\n", tlv->type);
> -                       break;
> -               }
> +                       default:
> +                               printf("Unrecognized type %d\n", tlv->type);
> +                               break;
> +                       }
>
> -               len -= tlvlen + sizeof(*tlv);
> -               assoc += tlvlen + sizeof(*tlv);
> -               tlv = (struct amp_tlv *) assoc;
> +                       len -= tlvlen + sizeof(*tlv);
> +                       assoc += tlvlen + sizeof(*tlv);
> +                       tlv = (struct amp_tlv *) assoc;
> +               } else
> +                       break;
>         }
>  }
> --
> 2.19.1

Please fix these:

Applying: hcidump: fixed AMP Assoc dump heap-over-flow
WARNING:LONG_LINE_STRING: line over 80 characters
#28: FILE: tools/parser/amp.c:88:
+                printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",

WARNING:LONG_LINE_STRING: line over 80 characters
#42: FILE: tools/parser/amp.c:94:
+                amp_dump_chanlist(level, tlv, "Preferred Chan List");

WARNING:LONG_LINE_STRING: line over 80 characters
#48: FILE: tools/parser/amp.c:98:
+                amp_dump_chanlist(level, tlv, "Connected Chan List");

WARNING:LONG_LINE_STRING: line over 80 characters
#70: FILE: tools/parser/amp.c:113:
+                printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",


-- 
Luiz Augusto von Dentz