[PATCH] monitor/rfcomm: Fix a potential memory access issue

[PATCH] monitor/rfcomm: Fix a potential memory access issue

[mcchou]

From: Miao-chen Chou <mcchou@chromium.org>

Packed structs have a default alignment of 1. If address of a member
is taken, the pointer value could be unaligned. Unaligned memory accesses
can result in a crash in some architectures.
---
 monitor/rfcomm.c | 2 +-
 monitor/rfcomm.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
index b32ad40..7c688af 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));
+} __attribute__ ((packed, aligned(2)));

 struct rfcomm_rls {
 	uint8_t dlci;
diff --git a/monitor/rfcomm.h b/monitor/rfcomm.h
index c157352..a8af484 100644
--- a/monitor/rfcomm.h
+++ b/monitor/rfcomm.h
@@ -77,4 +77,4 @@ struct rfcomm_pn {
 	uint16_t mtu;
 	uint8_t  max_retrans;
 	uint8_t  credits;
-} __attribute__((packed));
+} __attribute__((packed, aligned(2)));
--
2.8.0.rc3.226.g39d4020

Re: [PATCH] monitor/rfcomm: Fix a potential memory access issue

[Luiz Augusto von Dentz]

Hi,

On Sat, Nov 19, 2016 at 3:32 AM,  <mcchou@chromium.org> wrote:
> From: Miao-chen Chou <mcchou@chromium.org>
>
> Packed structs have a default alignment of 1. If address of a member
> is taken, the pointer value could be unaligned. Unaligned memory accesses
> can result in a crash in some architectures.

Afaik if it is byte aligned it shall never cause unaligned memory
accesses as the members are loaded byte a byte. which is what we want
here since it is a network PDU we don't want padding.

> ---
>  monitor/rfcomm.c | 2 +-
>  monitor/rfcomm.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
> index b32ad40..7c688af 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));
> +} __attribute__ ((packed, aligned(2)));
>
>  struct rfcomm_rls {
>         uint8_t dlci;
> diff --git a/monitor/rfcomm.h b/monitor/rfcomm.h
> index c157352..a8af484 100644
> --- a/monitor/rfcomm.h
> +++ b/monitor/rfcomm.h
> @@ -77,4 +77,4 @@ struct rfcomm_pn {
>         uint16_t mtu;
>         uint8_t  max_retrans;
>         uint8_t  credits;
> -} __attribute__((packed));
> +} __attribute__((packed, aligned(2)));
> --
> 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

Re: [PATCH] monitor/rfcomm: Fix a potential memory access issue

[Luiz Augusto von Dentz]

Hi,

On Mon, Nov 21, 2016 at 12:06 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Sat, Nov 19, 2016 at 3:32 AM,  <mcchou@chromium.org> wrote:
>> From: Miao-chen Chou <mcchou@chromium.org>
>>
>> Packed structs have a default alignment of 1. If address of a member
>> is taken, the pointer value could be unaligned. Unaligned memory accesses
>> can result in a crash in some architectures.
>
> Afaik if it is byte aligned it shall never cause unaligned memory
> accesses as the members are loaded byte a byte. which is what we want
> here since it is a network PDU we don't want padding.

Actually just to correct myself, packed can cause unaligned access,
but the solution is to use proper unaligned operation and not align
the struct which may introduce padding.

>> ---
>>  monitor/rfcomm.c | 2 +-
>>  monitor/rfcomm.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
>> index b32ad40..7c688af 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));
>> +} __attribute__ ((packed, aligned(2)));
>>
>>  struct rfcomm_rls {
>>         uint8_t dlci;
>> diff --git a/monitor/rfcomm.h b/monitor/rfcomm.h
>> index c157352..a8af484 100644
>> --- a/monitor/rfcomm.h
>> +++ b/monitor/rfcomm.h
>> @@ -77,4 +77,4 @@ struct rfcomm_pn {
>>         uint16_t mtu;
>>         uint8_t  max_retrans;
>>         uint8_t  credits;
>> -} __attribute__((packed));
>> +} __attribute__((packed, aligned(2)));
>> --
>> 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



-- 
Luiz Augusto von Dentz

[PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue

[mcchou]

From: Miao-chen Chou <mcchou@chromium.org>

This patch introduces a temp variable to prevent the access to an unaligned
struct member. Since the struct rfcomm_rpn is used from PDU, enforcing alignment
is not a solution due to padding.
---
 monitor/rfcomm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
index b32ad40..7f1f23e 100644
--- a/monitor/rfcomm.c
+++ b/monitor/rfcomm.c
@@ -199,6 +199,7 @@ 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;
+	uint16_t pm;

 	if (!l2cap_frame_get_u8(frame, &rpn.dlci))
 		return false;
@@ -235,8 +236,11 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
 		GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
 		rpn.xoff);

-	if (!l2cap_frame_get_le16(frame, &rpn.pm))
+	/* prevent unaligned memory access */
+	pm = rpn.pm;
+	if (!l2cap_frame_get_le16(frame, &pm))
 		return false;
+	rpn.pm = pm;

 	print_field("%*cpm 0x%04x", indent, ' ', rpn.pm);

@@ -265,6 +269,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 +289,11 @@ 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 */
+	mtu = pn.mtu;
+	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

Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue

[Luiz Augusto von Dentz]

Hi,

On Sat, Dec 3, 2016 at 4:08 AM,  <mcchou@chromium.org> wrote:
> From: Miao-chen Chou <mcchou@chromium.org>
>
> This patch introduces a temp variable to prevent the access to an unaligned
> struct member. Since the struct rfcomm_rpn is used from PDU, enforcing alignment
> is not a solution due to padding.
> ---
>  monitor/rfcomm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
> index b32ad40..7f1f23e 100644
> --- a/monitor/rfcomm.c
> +++ b/monitor/rfcomm.c
> @@ -199,6 +199,7 @@ 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;
> +       uint16_t pm;
>
>         if (!l2cap_frame_get_u8(frame, &rpn.dlci))
>                 return false;
> @@ -235,8 +236,11 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>                 GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
>                 rpn.xoff);
>
> -       if (!l2cap_frame_get_le16(frame, &rpn.pm))
> +       /* prevent unaligned memory access */
> +       pm = rpn.pm;
> +       if (!l2cap_frame_get_le16(frame, &pm))
>                 return false;
> +       rpn.pm = pm;

Not sure what was the intention here but if you were to introduce a
new variable for pm then we don't need rpn struct anymore.

>
>         print_field("%*cpm 0x%04x", indent, ' ', rpn.pm);
>
> @@ -265,6 +269,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 +289,11 @@ 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 */
> +       mtu = pn.mtu;
> +       if (!l2cap_frame_get_le16(frame, &mtu))
>                 return false;
> +       pn.mtu = mtu;

Similar problem here, but Im not exactly sure why this is a problem
since l2cap_frame_get_le16 does already takes care of unligned access.

>         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

Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue

[Miao-chen Chou]

Hi Luiz

The intention is to prevent accessing the addresses of pn.mtu and
rpn.pm, where the LLVM compiler complains about the potential
unaligned pointer value, and the use of temp variables ensures the
alignment. The error message are
monitor/rfcomm.c:241:36: error: taking address of packed member 'pm'
of class or structure 'rfcomm_rpn' may result in an unaligned pointer
value [-Werror,-Waddress-of-packed-member]

[18/1998]
        if (!l2cap_frame_get_le16(frame, &rpn.pm))
                                          ^~~~~~
monitor/rfcomm.c:293:36: error: taking address of packed member 'mtu'
of class or structure 'rfcomm_pn' may result in an unaligned pointer
value [-Werror,-Waddress-of-packed-member]
        if (!l2cap_frame_get_le16(frame, &pn.mtu))

Thanks,
Miao

On Wed, Dec 7, 2016 at 6:12 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Sat, Dec 3, 2016 at 4:08 AM,  <mcchou@chromium.org> wrote:
>> From: Miao-chen Chou <mcchou@chromium.org>
>>
>> This patch introduces a temp variable to prevent the access to an unaligned
>> struct member. Since the struct rfcomm_rpn is used from PDU, enforcing alignment
>> is not a solution due to padding.
>> ---
>>  monitor/rfcomm.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
>> index b32ad40..7f1f23e 100644
>> --- a/monitor/rfcomm.c
>> +++ b/monitor/rfcomm.c
>> @@ -199,6 +199,7 @@ 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;
>> +       uint16_t pm;
>>
>>         if (!l2cap_frame_get_u8(frame, &rpn.dlci))
>>                 return false;
>> @@ -235,8 +236,11 @@ static inline bool mcc_rpn(struct rfcomm_frame *rfcomm_frame, uint8_t indent)
>>                 GET_RPN_RTCI(rpn.io), GET_RPN_RTCO(rpn.io), rpn.xon,
>>                 rpn.xoff);
>>
>> -       if (!l2cap_frame_get_le16(frame, &rpn.pm))
>> +       /* prevent unaligned memory access */
>> +       pm = rpn.pm;
>> +       if (!l2cap_frame_get_le16(frame, &pm))
>>                 return false;
>> +       rpn.pm = pm;
>
> Not sure what was the intention here but if you were to introduce a
> new variable for pm then we don't need rpn struct anymore.
>
>>
>>         print_field("%*cpm 0x%04x", indent, ' ', rpn.pm);
>>
>> @@ -265,6 +269,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 +289,11 @@ 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 */
>> +       mtu = pn.mtu;
>> +       if (!l2cap_frame_get_le16(frame, &mtu))
>>                 return false;
>> +       pn.mtu = mtu;
>
> Similar problem here, but Im not exactly sure why this is a problem
> since l2cap_frame_get_le16 does already takes care of unligned access.
>
>>         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

Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue

[Johan Hedberg]

Hi Miao,

On Wed, Dec 07, 2016, Miao-chen Chou wrote:
> The intention is to prevent accessing the addresses of pn.mtu and
> rpn.pm, where the LLVM compiler complains about the potential
> unaligned pointer value, and the use of temp variables ensures the
> alignment. The error message are
> monitor/rfcomm.c:241:36: error: taking address of packed member 'pm'
> of class or structure 'rfcomm_rpn' may result in an unaligned pointer
> value [-Werror,-Waddress-of-packed-member]
> 
> [18/1998]
>         if (!l2cap_frame_get_le16(frame, &rpn.pm))
>                                           ^~~~~~
> monitor/rfcomm.c:293:36: error: taking address of packed member 'mtu'
> of class or structure 'rfcomm_pn' may result in an unaligned pointer
> value [-Werror,-Waddress-of-packed-member]
>         if (!l2cap_frame_get_le16(frame, &pn.mtu))

It seems to me like there's no point in mcc_rpn() using the packed
rfcomm_rpn struct, since what it really wants is normal properly aligned
stack variables. So a more appropriate fix is probably to remote the
rfcomm_rpn struct usage and instead just declare the necessary "normal"
variables to fetch the necessary values from the frame. Another, perhaps
better alternative, is to use a struct rfcomm_rpn pointer to frame->data
and use l2cap_frame_pull() to advance the data pointer, i.e. something
like:

	struct rfcomm_rpn *rpn;

	if (frame->size < sizeof(*rpn))
		return false;

	rpn = frame->data;
	l2cap_frame_pull(frame, frame, sizeof(*rpn))

	...print content of rpn, utilizing get_le16() etc...

The downside of the above is that if you get an incomplete frame we
don't decode even the parts that we did receive. I'm not sure what's the
preferred policy with btmon decoding.

Johan

Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue

[Miao-chen Chou]

Hi Johan and Luiz,

I agree with Johan's comment on the rfcomm_rpn struct. struct
rfcomm_rpn is only used on mcc_rpn(), and it causes the potential
alignment issue. Removing it and replacing the use with normal stack
variables seems to be a good way to go. Since the related patch
"gobex: Fix a compilation error for the compatibility with LLVM" has
been adopted. I will upload the next patch in a new thread. Thanks.

-Miao

On Wed, Dec 7, 2016 at 1:07 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Miao,
>
> On Wed, Dec 07, 2016, Miao-chen Chou wrote:
>> The intention is to prevent accessing the addresses of pn.mtu and
>> rpn.pm, where the LLVM compiler complains about the potential
>> unaligned pointer value, and the use of temp variables ensures the
>> alignment. The error message are
>> monitor/rfcomm.c:241:36: error: taking address of packed member 'pm'
>> of class or structure 'rfcomm_rpn' may result in an unaligned pointer
>> value [-Werror,-Waddress-of-packed-member]
>>
>> [18/1998]
>>         if (!l2cap_frame_get_le16(frame, &rpn.pm))
>>                                           ^~~~~~
>> monitor/rfcomm.c:293:36: error: taking address of packed member 'mtu'
>> of class or structure 'rfcomm_pn' may result in an unaligned pointer
>> value [-Werror,-Waddress-of-packed-member]
>>         if (!l2cap_frame_get_le16(frame, &pn.mtu))
>
> It seems to me like there's no point in mcc_rpn() using the packed
> rfcomm_rpn struct, since what it really wants is normal properly aligned
> stack variables. So a more appropriate fix is probably to remote the
> rfcomm_rpn struct usage and instead just declare the necessary "normal"
> variables to fetch the necessary values from the frame. Another, perhaps
> better alternative, is to use a struct rfcomm_rpn pointer to frame->data
> and use l2cap_frame_pull() to advance the data pointer, i.e. something
> like:
>
>         struct rfcomm_rpn *rpn;
>
>         if (frame->size < sizeof(*rpn))
>                 return false;
>
>         rpn = frame->data;
>         l2cap_frame_pull(frame, frame, sizeof(*rpn))
>
>         ...print content of rpn, utilizing get_le16() etc...
>
> The downside of the above is that if you get an incomplete frame we
> don't decode even the parts that we did receive. I'm not sure what's the
> preferred policy with btmon decoding.
>
> Johan