* [PATCH v2] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
@ 2016-12-08 1:35 mcchou
2016-12-09 9:52 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 7+ messages in thread
From: mcchou @ 2016-12-08 1:35 UTC (permalink / raw)
To: linux-bluetooth; +Cc: luiz.von.dentz, josephsih, johan.hedberg, Miao-chen Chou
From: Miao-chen Chou <mcchou@chromium.org>
This patch replaces the use of struct rfcomm_rpn with local variables in
mmc_rpn() to prevent the access to an unaligned struct member. Since struct
rfcomm_rpn is only used in mmc_rpn(), its definition is removed. This patch
also introduces a temp variable in mcc_pn() to prevent unaligned access.
---
monitor/rfcomm.c | 48 +++++++++++++++++++++---------------------------
1 file changed, 21 insertions(+), 27 deletions(-)
diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
index b32ad40..6b9d355 100644
--- a/monitor/rfcomm.c
+++ b/monitor/rfcomm.c
@@ -98,16 +98,6 @@ struct rfcomm_lmsc {
uint8_t break_sig;
} __attribute__((packed));
-struct rfcomm_rpn {
- uint8_t dlci;
- uint8_t bit_rate;
- uint8_t parity;
- uint8_t io;
- uint8_t xon;
- uint8_t xoff;
- uint16_t pm;
-} __attribute__ ((packed));
-
struct rfcomm_rls {
uint8_t dlci;
uint8_t error;
@@ -198,47 +188,48 @@ done:
static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
{
struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
- struct rfcomm_rpn rpn;
+ uint8_t dlci, bit_rate, parity, io, xon, xoff;
+ uint16_t pm;
- if (!l2cap_frame_get_u8(frame, &rpn.dlci))
+ if (!l2cap_frame_get_u8(frame, &dlci))
return false;
- print_field("%*cdlci %d", indent, ' ', RFCOMM_GET_DLCI(rpn.dlci));
+ print_field("%*cdlci %d", indent, ' ', RFCOMM_GET_DLCI(dlci));
if (frame->size < 7)
goto done;
/* port value octets (optional) */
- if (!l2cap_frame_get_u8(frame, &rpn.bit_rate))
+ if (!l2cap_frame_get_u8(frame, &bit_rate))
return false;
- if (!l2cap_frame_get_u8(frame, &rpn.parity))
+ if (!l2cap_frame_get_u8(frame, &parity))
return false;
- if (!l2cap_frame_get_u8(frame, &rpn.io))
+ if (!l2cap_frame_get_u8(frame, &io))
return false;
print_field("%*cbr %d db %d sb %d p %d pt %d xi %d xo %d", indent, ' ',
- rpn.bit_rate, GET_RPN_DB(rpn.parity), GET_RPN_SB(rpn.parity),
- GET_RPN_PARITY(rpn.parity), GET_RPN_PTYPE(rpn.parity),
- GET_RPN_XIN(rpn.io), GET_RPN_XOUT(rpn.io));
+ bit_rate, GET_RPN_DB(parity), GET_RPN_SB(parity),
+ GET_RPN_PARITY(parity), GET_RPN_PTYPE(parity),
+ GET_RPN_XIN(io), GET_RPN_XOUT(io));
- if (!l2cap_frame_get_u8(frame, &rpn.xon))
+ if (!l2cap_frame_get_u8(frame, &xon))
return false;
- if (!l2cap_frame_get_u8(frame, &rpn.xoff))
+ if (!l2cap_frame_get_u8(frame, &xoff))
return false;
print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
- indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
- GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
- rpn.xoff);
+ indent, ' ', GET_RPN_RTRI(io), GET_RPN_RTRO(io),
+ GET_RPN_RTCI(io), GET_RPN_RTCO(io), xon, xoff);
- if (!l2cap_frame_get_le16(frame, &rpn.pm))
+ /* prevent unaligned memory access */
+ if (!l2cap_frame_get_le16(frame, &pm))
return false;
- print_field("%*cpm 0x%04x", indent, ' ', rpn.pm);
+ print_field("%*cpm 0x%04x", indent, ' ', pm);
done:
return true;
@@ -265,6 +256,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
{
struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
struct rfcomm_pn pn;
+ uint16_t mtu;
/* rfcomm_pn struct is defined in rfcomm.h */
@@ -284,8 +276,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
return false;
- if (!l2cap_frame_get_le16(frame, &pn.mtu))
+ /* prevent unaligned memory access */
+ if (!l2cap_frame_get_le16(frame, &mtu))
return false;
+ pn.mtu = mtu;
if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
return false;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-08 1:35 [PATCH v2] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM mcchou
@ 2016-12-09 9:52 ` Luiz Augusto von Dentz
2016-12-09 20:59 ` [PATCH v3] " mcchou
0 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2016-12-09 9:52 UTC (permalink / raw)
To: Miao-chen Chou
Cc: linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz,
josephsih, Johan Hedberg
Hi,
On Thu, Dec 8, 2016 at 3:35 AM, <mcchou@chromium.org> wrote:
> From: Miao-chen Chou <mcchou@chromium.org>
>
> This patch replaces the use of struct rfcomm_rpn with local variables in
> mmc_rpn() to prevent the access to an unaligned struct member. Since struct
> rfcomm_rpn is only used in mmc_rpn(), its definition is removed. This patch
> also introduces a temp variable in mcc_pn() to prevent unaligned access.
> ---
> monitor/rfcomm.c | 48 +++++++++++++++++++++---------------------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
> index b32ad40..6b9d355 100644
> --- a/monitor/rfcomm.c
> +++ b/monitor/rfcomm.c
> @@ -98,16 +98,6 @@ struct rfcomm_lmsc {
> uint8_t break_sig;
> } __attribute__((packed));
>
> -struct rfcomm_rpn {
> - uint8_t dlci;
> - uint8_t bit_rate;
> - uint8_t parity;
> - uint8_t io;
> - uint8_t xon;
> - uint8_t xoff;
> - uint16_t pm;
> -} __attribute__ ((packed));
> -
I guess it would be cleaner if you just remove the packed from these
structs since we are not using it to store the raw PDU the padding can
be adjusted by the compiler.
> struct rfcomm_rls {
> uint8_t dlci;
> uint8_t error;
> @@ -198,47 +188,48 @@ done:
> static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
> {
> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
> - struct rfcomm_rpn rpn;
> + uint8_t dlci, bit_rate, parity, io, xon, xoff;
> + uint16_t pm;
>
> - if (!l2cap_frame_get_u8(frame, &rpn.dlci))
> + if (!l2cap_frame_get_u8(frame, &dlci))
> return false;
>
> - print_field("%*cdlci %d", indent, ' ', RFCOMM_GET_DLCI(rpn.dlci));
> + print_field("%*cdlci %d", indent, ' ', RFCOMM_GET_DLCI(dlci));
>
> if (frame->size < 7)
> goto done;
>
> /* port value octets (optional) */
>
> - if (!l2cap_frame_get_u8(frame, &rpn.bit_rate))
> + if (!l2cap_frame_get_u8(frame, &bit_rate))
> return false;
>
> - if (!l2cap_frame_get_u8(frame, &rpn.parity))
> + if (!l2cap_frame_get_u8(frame, &parity))
> return false;
>
> - if (!l2cap_frame_get_u8(frame, &rpn.io))
> + if (!l2cap_frame_get_u8(frame, &io))
> return false;
>
> print_field("%*cbr %d db %d sb %d p %d pt %d xi %d xo %d", indent, ' ',
> - rpn.bit_rate, GET_RPN_DB(rpn.parity), GET_RPN_SB(rpn.parity),
> - GET_RPN_PARITY(rpn.parity), GET_RPN_PTYPE(rpn.parity),
> - GET_RPN_XIN(rpn.io), GET_RPN_XOUT(rpn.io));
> + bit_rate, GET_RPN_DB(parity), GET_RPN_SB(parity),
> + GET_RPN_PARITY(parity), GET_RPN_PTYPE(parity),
> + GET_RPN_XIN(io), GET_RPN_XOUT(io));
>
> - if (!l2cap_frame_get_u8(frame, &rpn.xon))
> + if (!l2cap_frame_get_u8(frame, &xon))
> return false;
>
> - if (!l2cap_frame_get_u8(frame, &rpn.xoff))
> + if (!l2cap_frame_get_u8(frame, &xoff))
> return false;
>
> print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
> - indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
> - GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
> - rpn.xoff);
> + indent, ' ', GET_RPN_RTRI(io), GET_RPN_RTRO(io),
> + GET_RPN_RTCI(io), GET_RPN_RTCO(io), xon, xoff);
>
> - if (!l2cap_frame_get_le16(frame, &rpn.pm))
> + /* prevent unaligned memory access */
> + if (!l2cap_frame_get_le16(frame, &pm))
> return false;
>
> - print_field("%*cpm 0x%04x", indent, ' ', rpn.pm);
> + print_field("%*cpm 0x%04x", indent, ' ', pm);
>
> done:
> return true;
> @@ -265,6 +256,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
> {
> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
> struct rfcomm_pn pn;
> + uint16_t mtu;
>
> /* rfcomm_pn struct is defined in rfcomm.h */
>
> @@ -284,8 +276,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
> if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
> return false;
>
> - if (!l2cap_frame_get_le16(frame, &pn.mtu))
> + /* prevent unaligned memory access */
> + if (!l2cap_frame_get_le16(frame, &mtu))
> return false;
> + pn.mtu = mtu;
>
> if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
> return false;
> --
> 2.8.0.rc3.226.g39d4020
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-09 9:52 ` Luiz Augusto von Dentz
@ 2016-12-09 20:59 ` mcchou
2016-12-13 21:34 ` Miao-chen Chou
0 siblings, 1 reply; 7+ messages in thread
From: mcchou @ 2016-12-09 20:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: luiz.von.dentz, josephsih, Miao-chen Chou
From: Miao-chen Chou <mcchou@chromium.org>
This patch removes "packed" attribute from the definition of struct rfcomm_rpn
to prevent the access to an unaligned struct member in mmc_rpn(). This patch
also introduces a temp variable in mcc_pn() to prevent unaligned access without
touching the definition of struct rfcomm_pn, since struct rfcomm_pn is used as
a PDU.
---
monitor/rfcomm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
index b32ad40..08f3e36 100644
--- a/monitor/rfcomm.c
+++ b/monitor/rfcomm.c
@@ -106,7 +106,7 @@ struct rfcomm_rpn {
uint8_t xon;
uint8_t xoff;
uint16_t pm;
-} __attribute__ ((packed));
+};
struct rfcomm_rls {
uint8_t dlci;
@@ -232,8 +232,7 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
- GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
- rpn.xoff);
+ GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon, rpn.xoff);
if (!l2cap_frame_get_le16(frame, &rpn.pm))
return false;
@@ -265,6 +264,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
{
struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
struct rfcomm_pn pn;
+ uint16_t mtu;
/* rfcomm_pn struct is defined in rfcomm.h */
@@ -284,8 +284,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
return false;
- if (!l2cap_frame_get_le16(frame, &pn.mtu))
+ /* prevent unaligned memory access */
+ if (!l2cap_frame_get_le16(frame, &mtu))
return false;
+ pn.mtu = mtu;
if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
return false;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-09 20:59 ` [PATCH v3] " mcchou
@ 2016-12-13 21:34 ` Miao-chen Chou
2016-12-15 12:05 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 7+ messages in thread
From: Miao-chen Chou @ 2016-12-13 21:34 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Von Dentz, Luiz, josephsih, Miao-chen Chou
Hello Luiz,
Is there any comment on this patch?
Thanks,
Miao
On Sat, Dec 10, 2016 at 4:59 AM, <mcchou@chromium.org> wrote:
> From: Miao-chen Chou <mcchou@chromium.org>
>
> This patch removes "packed" attribute from the definition of struct rfcomm_rpn
> to prevent the access to an unaligned struct member in mmc_rpn(). This patch
> also introduces a temp variable in mcc_pn() to prevent unaligned access without
> touching the definition of struct rfcomm_pn, since struct rfcomm_pn is used as
> a PDU.
> ---
> monitor/rfcomm.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
> index b32ad40..08f3e36 100644
> --- a/monitor/rfcomm.c
> +++ b/monitor/rfcomm.c
> @@ -106,7 +106,7 @@ struct rfcomm_rpn {
> uint8_t xon;
> uint8_t xoff;
> uint16_t pm;
> -} __attribute__ ((packed));
> +};
>
> struct rfcomm_rls {
> uint8_t dlci;
> @@ -232,8 +232,7 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>
> print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
> indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
> - GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
> - rpn.xoff);
> + GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon, rpn.xoff);
>
> if (!l2cap_frame_get_le16(frame, &rpn.pm))
> return false;
> @@ -265,6 +264,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
> {
> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
> struct rfcomm_pn pn;
> + uint16_t mtu;
>
> /* rfcomm_pn struct is defined in rfcomm.h */
>
> @@ -284,8 +284,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
> if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
> return false;
>
> - if (!l2cap_frame_get_le16(frame, &pn.mtu))
> + /* prevent unaligned memory access */
> + if (!l2cap_frame_get_le16(frame, &mtu))
> return false;
> + pn.mtu = mtu;
>
> if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
> return false;
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-13 21:34 ` Miao-chen Chou
@ 2016-12-15 12:05 ` Luiz Augusto von Dentz
2016-12-19 19:13 ` Miao-chen Chou
0 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2016-12-15 12:05 UTC (permalink / raw)
To: Miao-chen Chou
Cc: linux-bluetooth@vger.kernel.org, Von Dentz, Luiz, josephsih
Hi,
On Tue, Dec 13, 2016 at 11:34 PM, Miao-chen Chou <mcchou@chromium.org> wrote:
> Hello Luiz,
>
> Is there any comment on this patch?
Ive sent a similar patch removing the remaining packed attribute from
the RFCOMM, not sure if you have seen it? There is actually no reason
to keep them packed since it is not used to stored the raw packets, so
if you can please test the patch and let me know if that fixes the
LLVM problem.
> Thanks,
> Miao
>
> On Sat, Dec 10, 2016 at 4:59 AM, <mcchou@chromium.org> wrote:
>> From: Miao-chen Chou <mcchou@chromium.org>
>>
>> This patch removes "packed" attribute from the definition of struct rfcomm_rpn
>> to prevent the access to an unaligned struct member in mmc_rpn(). This patch
>> also introduces a temp variable in mcc_pn() to prevent unaligned access without
>> touching the definition of struct rfcomm_pn, since struct rfcomm_pn is used as
>> a PDU.
>> ---
>> monitor/rfcomm.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
>> index b32ad40..08f3e36 100644
>> --- a/monitor/rfcomm.c
>> +++ b/monitor/rfcomm.c
>> @@ -106,7 +106,7 @@ struct rfcomm_rpn {
>> uint8_t xon;
>> uint8_t xoff;
>> uint16_t pm;
>> -} __attribute__ ((packed));
>> +};
>>
>> struct rfcomm_rls {
>> uint8_t dlci;
>> @@ -232,8 +232,7 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>
>> print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
>> indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
>> - GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
>> - rpn.xoff);
>> + GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon, rpn.xoff);
>>
>> if (!l2cap_frame_get_le16(frame, &rpn.pm))
>> return false;
>> @@ -265,6 +264,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>> {
>> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
>> struct rfcomm_pn pn;
>> + uint16_t mtu;
>>
>> /* rfcomm_pn struct is defined in rfcomm.h */
>>
>> @@ -284,8 +284,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>> if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
>> return false;
>>
>> - if (!l2cap_frame_get_le16(frame, &pn.mtu))
>> + /* prevent unaligned memory access */
>> + if (!l2cap_frame_get_le16(frame, &mtu))
>> return false;
>> + pn.mtu = mtu;
>>
>> if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
>> return false;
>> --
>> 2.8.0.rc3.226.g39d4020
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-15 12:05 ` Luiz Augusto von Dentz
@ 2016-12-19 19:13 ` Miao-chen Chou
2016-12-20 19:12 ` Miao-chen Chou
0 siblings, 1 reply; 7+ messages in thread
From: Miao-chen Chou @ 2016-12-19 19:13 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth@vger.kernel.org, Von Dentz, Luiz, josephsih
Hi Luiz,
Yes, I saw that patch
(http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=452024ba83452eee5accdbe5506ab3683cd4790c).
I found that 452024 doesn't fix the issue, since it doesn't remove the
packed attribute from rfcomm_rpn structure in monitor/rfcomm.c.
Therefore, we still need this patch.
Thanks,
Miao
On Thu, Dec 15, 2016 at 4:05 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Tue, Dec 13, 2016 at 11:34 PM, Miao-chen Chou <mcchou@chromium.org> wrote:
>> Hello Luiz,
>>
>> Is there any comment on this patch?
>
> Ive sent a similar patch removing the remaining packed attribute from
> the RFCOMM, not sure if you have seen it? There is actually no reason
> to keep them packed since it is not used to stored the raw packets, so
> if you can please test the patch and let me know if that fixes the
> LLVM problem.
>
>> Thanks,
>> Miao
>>
>> On Sat, Dec 10, 2016 at 4:59 AM, <mcchou@chromium.org> wrote:
>>> From: Miao-chen Chou <mcchou@chromium.org>
>>>
>>> This patch removes "packed" attribute from the definition of struct rfcomm_rpn
>>> to prevent the access to an unaligned struct member in mmc_rpn(). This patch
>>> also introduces a temp variable in mcc_pn() to prevent unaligned access without
>>> touching the definition of struct rfcomm_pn, since struct rfcomm_pn is used as
>>> a PDU.
>>> ---
>>> monitor/rfcomm.c | 10 ++++++----
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
>>> index b32ad40..08f3e36 100644
>>> --- a/monitor/rfcomm.c
>>> +++ b/monitor/rfcomm.c
>>> @@ -106,7 +106,7 @@ struct rfcomm_rpn {
>>> uint8_t xon;
>>> uint8_t xoff;
>>> uint16_t pm;
>>> -} __attribute__ ((packed));
>>> +};
>>>
>>> struct rfcomm_rls {
>>> uint8_t dlci;
>>> @@ -232,8 +232,7 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>>
>>> print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
>>> indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
>>> - GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
>>> - rpn.xoff);
>>> + GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon, rpn.xoff);
>>>
>>> if (!l2cap_frame_get_le16(frame, &rpn.pm))
>>> return false;
>>> @@ -265,6 +264,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>> {
>>> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
>>> struct rfcomm_pn pn;
>>> + uint16_t mtu;
>>>
>>> /* rfcomm_pn struct is defined in rfcomm.h */
>>>
>>> @@ -284,8 +284,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>> if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
>>> return false;
>>>
>>> - if (!l2cap_frame_get_le16(frame, &pn.mtu))
>>> + /* prevent unaligned memory access */
>>> + if (!l2cap_frame_get_le16(frame, &mtu))
>>> return false;
>>> + pn.mtu = mtu;
>>>
>>> if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
>>> return false;
>>> --
>>> 2.8.0.rc3.226.g39d4020
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM
2016-12-19 19:13 ` Miao-chen Chou
@ 2016-12-20 19:12 ` Miao-chen Chou
0 siblings, 0 replies; 7+ messages in thread
From: Miao-chen Chou @ 2016-12-20 19:12 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth@vger.kernel.org, Von Dentz, Luiz, josephsih
Hi,
This patch also include the fix of the unaligned member access to
struct rfcomm_pn in function mcc_pn(). Since struct rfcomm_pn is used
for PDU, the packed attribute cannot be removed from it.
Thanks,
Miao
On Mon, Dec 19, 2016 at 11:13 AM, Miao-chen Chou <mcchou@chromium.org> wrote:
> Hi Luiz,
>
> Yes, I saw that patch
> (http://git.kernel.org/cgit/bluetooth/bluez.git/commit/?id=452024ba83452eee5accdbe5506ab3683cd4790c).
> I found that 452024 doesn't fix the issue, since it doesn't remove the
> packed attribute from rfcomm_rpn structure in monitor/rfcomm.c.
> Therefore, we still need this patch.
>
> Thanks,
> Miao
>
> On Thu, Dec 15, 2016 at 4:05 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi,
>>
>> On Tue, Dec 13, 2016 at 11:34 PM, Miao-chen Chou <mcchou@chromium.org> wrote:
>>> Hello Luiz,
>>>
>>> Is there any comment on this patch?
>>
>> Ive sent a similar patch removing the remaining packed attribute from
>> the RFCOMM, not sure if you have seen it? There is actually no reason
>> to keep them packed since it is not used to stored the raw packets, so
>> if you can please test the patch and let me know if that fixes the
>> LLVM problem.
>>
>>> Thanks,
>>> Miao
>>>
>>> On Sat, Dec 10, 2016 at 4:59 AM, <mcchou@chromium.org> wrote:
>>>> From: Miao-chen Chou <mcchou@chromium.org>
>>>>
>>>> This patch removes "packed" attribute from the definition of struct rfcomm_rpn
>>>> to prevent the access to an unaligned struct member in mmc_rpn(). This patch
>>>> also introduces a temp variable in mcc_pn() to prevent unaligned access without
>>>> touching the definition of struct rfcomm_pn, since struct rfcomm_pn is used as
>>>> a PDU.
>>>> ---
>>>> monitor/rfcomm.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
>>>> index b32ad40..08f3e36 100644
>>>> --- a/monitor/rfcomm.c
>>>> +++ b/monitor/rfcomm.c
>>>> @@ -106,7 +106,7 @@ struct rfcomm_rpn {
>>>> uint8_t xon;
>>>> uint8_t xoff;
>>>> uint16_t pm;
>>>> -} __attribute__ ((packed));
>>>> +};
>>>>
>>>> struct rfcomm_rls {
>>>> uint8_t dlci;
>>>> @@ -232,8 +232,7 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>>>
>>>> print_field("%*crtri %d rtro %d rtci %d rtco %d xon %d xoff %d",
>>>> indent, ' ', GET_RPN_RTRI(rpn.io), GET_RPN_RTRO(rpn.io),
>>>> - GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
>>>> - rpn.xoff);
>>>> + GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon, rpn.xoff);
>>>>
>>>> if (!l2cap_frame_get_le16(frame, &rpn.pm))
>>>> return false;
>>>> @@ -265,6 +264,7 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>>> {
>>>> struct l2cap_frame *frame = &rfcomm_frame->l2cap_frame;
>>>> struct rfcomm_pn pn;
>>>> + uint16_t mtu;
>>>>
>>>> /* rfcomm_pn struct is defined in rfcomm.h */
>>>>
>>>> @@ -284,8 +284,10 @@ static inline bool mcc_pn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>>> if (!l2cap_frame_get_u8(frame, &pn.ack_timer))
>>>> return false;
>>>>
>>>> - if (!l2cap_frame_get_le16(frame, &pn.mtu))
>>>> + /* prevent unaligned memory access */
>>>> + if (!l2cap_frame_get_le16(frame, &mtu))
>>>> return false;
>>>> + pn.mtu = mtu;
>>>>
>>>> if (!l2cap_frame_get_u8(frame, &pn.max_retrans))
>>>> return false;
>>>> --
>>>> 2.8.0.rc3.226.g39d4020
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-12-20 19:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-08 1:35 [PATCH v2] monitor/rfcomm: Fix a potential memory access issue for compatibility with LLVM mcchou
2016-12-09 9:52 ` Luiz Augusto von Dentz
2016-12-09 20:59 ` [PATCH v3] " mcchou
2016-12-13 21:34 ` Miao-chen Chou
2016-12-15 12:05 ` Luiz Augusto von Dentz
2016-12-19 19:13 ` Miao-chen Chou
2016-12-20 19:12 ` Miao-chen Chou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).