* [PATCH] monitor/rfcomm: Fix a potential memory access issue @ 2016-11-19 1:32 mcchou 2016-11-21 10:06 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: mcchou @ 2016-11-19 1:32 UTC (permalink / raw) To: linux-bluetooth; +Cc: luiz.von.dentz, josephsih, Miao-chen Chou 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] monitor/rfcomm: Fix a potential memory access issue 2016-11-19 1:32 [PATCH] monitor/rfcomm: Fix a potential memory access issue mcchou @ 2016-11-21 10:06 ` Luiz Augusto von Dentz 2016-11-21 10:18 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2016-11-21 10:06 UTC (permalink / raw) To: mcchou; +Cc: linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] monitor/rfcomm: Fix a potential memory access issue 2016-11-21 10:06 ` Luiz Augusto von Dentz @ 2016-11-21 10:18 ` Luiz Augusto von Dentz 2016-12-03 2:08 ` [PATCH 2/2] " mcchou 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2016-11-21 10:18 UTC (permalink / raw) To: mcchou; +Cc: linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue 2016-11-21 10:18 ` Luiz Augusto von Dentz @ 2016-12-03 2:08 ` mcchou 2016-12-07 14:12 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: mcchou @ 2016-12-03 2:08 UTC (permalink / raw) To: linux-bluetooth; +Cc: luiz.von.dentz, josephsih, Miao-chen Chou 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue 2016-12-03 2:08 ` [PATCH 2/2] " mcchou @ 2016-12-07 14:12 ` Luiz Augusto von Dentz 2016-12-07 20:27 ` Miao-chen Chou 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2016-12-07 14:12 UTC (permalink / raw) To: Miao-chen Chou Cc: linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue 2016-12-07 14:12 ` Luiz Augusto von Dentz @ 2016-12-07 20:27 ` Miao-chen Chou 2016-12-07 21:07 ` Johan Hedberg 0 siblings, 1 reply; 8+ messages in thread From: Miao-chen Chou @ 2016-12-07 20:27 UTC (permalink / raw) To: Luiz Augusto von Dentz Cc: linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue 2016-12-07 20:27 ` Miao-chen Chou @ 2016-12-07 21:07 ` Johan Hedberg 2016-12-07 23:20 ` Miao-chen Chou 0 siblings, 1 reply; 8+ messages in thread From: Johan Hedberg @ 2016-12-07 21:07 UTC (permalink / raw) To: Miao-chen Chou Cc: Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] monitor/rfcomm: Fix a potential memory access issue 2016-12-07 21:07 ` Johan Hedberg @ 2016-12-07 23:20 ` Miao-chen Chou 0 siblings, 0 replies; 8+ messages in thread From: Miao-chen Chou @ 2016-12-07 23:20 UTC (permalink / raw) To: Miao-chen Chou, Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org, Luiz Augusto Von Dentz, josephsih 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-07 23:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-19 1:32 [PATCH] monitor/rfcomm: Fix a potential memory access issue mcchou 2016-11-21 10:06 ` Luiz Augusto von Dentz 2016-11-21 10:18 ` Luiz Augusto von Dentz 2016-12-03 2:08 ` [PATCH 2/2] " mcchou 2016-12-07 14:12 ` Luiz Augusto von Dentz 2016-12-07 20:27 ` Miao-chen Chou 2016-12-07 21:07 ` Johan Hedberg 2016-12-07 23:20 ` 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).