Bugs were simple buffer overflow that was actually discovered already couple years ago
by op7ic:https://www.spinics.net/lists/linux-bluetooth/msg68892.html
Caused by missing boundary checks before accessing.
Cho, Yu-Chen (3):
hcidump: fixed AMP Assoc dump heap-over-flow
hcidump:fixed hci frame dump stack-buffer-overflow
hcidump: Fix set_ext_ctrl() global buffer overflow
tools/parser/amp.c | 65 +++++++++++++++++++++++---------------------
tools/parser/hci.c | 3 ++
tools/parser/l2cap.c | 2 +-
3 files changed, 38 insertions(+), 32 deletions(-)
--
2.19.1
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 <[email protected]>
---
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
hci_dump() didn't check the length of frame, and it would be
a stack-buffer-overflow error.
Signed-off-by: Cho, Yu-Chen <[email protected]>
---
tools/parser/hci.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/parser/hci.c b/tools/parser/hci.c
index 8c7bd2581..4e6c36040 100644
--- a/tools/parser/hci.c
+++ b/tools/parser/hci.c
@@ -4107,6 +4107,9 @@ void hci_dump(int level, struct frame *frm)
frm->ptr++; frm->len--;
+ if (frm->len == 0)
+ return;
+
switch (type) {
case HCI_COMMAND_PKT:
command_dump(level, frm);
--
2.19.1
Fix set_ext_ctrl() global buffer overflow.
Signed-off-by: Cho, Yu-Chen <[email protected]>
---
tools/parser/l2cap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/parser/l2cap.c b/tools/parser/l2cap.c
index a05796482..5daefcbaa 100644
--- a/tools/parser/l2cap.c
+++ b/tools/parser/l2cap.c
@@ -56,7 +56,7 @@ typedef struct {
uint8_t mode;
uint8_t ext_ctrl;
} cid_info;
-#define CID_TABLE_SIZE 20
+#define CID_TABLE_SIZE 32
static cid_info cid_table[2][CID_TABLE_SIZE];
--
2.19.1
Hi,
On Wed, Oct 31, 2018 at 11:08 AM Cho, Yu-Chen <[email protected]> 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 <[email protected]>
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
Hi,
On Wed, Oct 31, 2018 at 10:39 AM Cho, Yu-Chen <[email protected]> wrote:
>
> hci_dump() didn't check the length of frame, and it would be
> a stack-buffer-overflow error.
>
> Signed-off-by: Cho, Yu-Chen <[email protected]>
> ---
> tools/parser/hci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/parser/hci.c b/tools/parser/hci.c
> index 8c7bd2581..4e6c36040 100644
> --- a/tools/parser/hci.c
> +++ b/tools/parser/hci.c
> @@ -4107,6 +4107,9 @@ void hci_dump(int level, struct frame *frm)
>
> frm->ptr++; frm->len--;
>
> + if (frm->len == 0)
> + return;
> +
> switch (type) {
> case HCI_COMMAND_PKT:
> command_dump(level, frm);
> --
> 2.19.1
Applied.
--
Luiz Augusto von Dentz
Hi,
On Wed, Oct 31, 2018 at 10:38 AM Cho, Yu-Chen <[email protected]> wrote:
>
> Fix set_ext_ctrl() global buffer overflow.
>
> Signed-off-by: Cho, Yu-Chen <[email protected]>
> ---
> tools/parser/l2cap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/parser/l2cap.c b/tools/parser/l2cap.c
> index a05796482..5daefcbaa 100644
> --- a/tools/parser/l2cap.c
> +++ b/tools/parser/l2cap.c
> @@ -56,7 +56,7 @@ typedef struct {
> uint8_t mode;
> uint8_t ext_ctrl;
> } cid_info;
> -#define CID_TABLE_SIZE 20
> +#define CID_TABLE_SIZE 32
>
> static cid_info cid_table[2][CID_TABLE_SIZE];
>
> --
> 2.19.1
Applied, thanks.
--
Luiz Augusto von Dentz
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 <[email protected]>
---
tools/parser/amp.c | 69 +++++++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/tools/parser/amp.c b/tools/parser/amp.c
index 158ca4a75..420099c90 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,54 @@ 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: ");
+ printf("%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: ");
+ printf("%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