Hi, On Wed, Dec 21, 2016 at 9:26 PM, Miao-chen Chou <mcchou@xxxxxxxxxx> wrote: > Hi Luiz, > > The unaligned error is raised when trying to access the address of mtu > of a packed struct where mtu's size is 2 bytes; which means accessing > the address of a packed member whose size is greater than 1 byte can > raised this error. So it is not true that there is no error as far as > the member is not accessed via void *(see the following function > signature). > > static inline bool l2cap_frame_get_le16(struct l2cap_frame *frame, > uint16_t *value) > > Here are the errors generated. The flags are "Werror" and > "Waddress-of-packed-member". I will also add these two errors into the > commit message. > > monitor/rfcomm.c:238: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] > if (!l2cap_frame_get_le16(frame, &rpn.pm)) > monitor/rfcomm.c:287: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)) Ive only found the following bug in GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628 But there doesn't seem to have any option such as -Waddress-of-packed-member but it seems valid as explained in the bug. So we got to remove any use of packed struct that don't represent the PDU, iirc we did choose to parse individual members so we could tell exactly where it PDU failed to parse but that is not done in other layers which do use packed structs but they don't take addresses of members so they are safe. Imo we should not resort to packed in the monitor, specially since gcc don't seem to have a flag to catch addresses of packed member, for non-monitor it might be fine because they don't need to be decoding everything. > Thanks, > Miao > > On Wed, Dec 21, 2016 at 6:03 AM, Luiz Augusto von Dentz > <luiz.dentz@xxxxxxxxx> wrote: >> Hi, >> >> On Tue, Dec 20, 2016 at 9:50 PM, <mcchou@xxxxxxxxxxxx> wrote: >>> From: Miao-chen Chou <mcchou@xxxxxxxxxxxx> >>> >>> 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. >>> --- >>> v4 removes the unrelated change on the code format. >>> >>> monitor/rfcomm.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c >>> index b32ad40..4bd549c 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; >>> @@ -265,6 +265,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 +285,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; >> >> Interesting, isn't this causing unaligned access anyway since it is >> assigning to pn.mtu which is packed? Or this is fine as far as it is >> not accessed via void *? Btw, please add the compiler output and flags >> it is being used, it is quite possible we can replicate the same >> errors on gcc or perhaps it is missing some flags used in gcc to make >> this aligned? >> >>> 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@xxxxxxxxxxxxxxx >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Luiz Augusto von Dentz -- Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
