Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event
Subsystems (3)
cover Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event
From: Alexander Grund <[email protected]>
This basically fixes the memcpy(dst, src, strlen(src)) where dst is a fixed size array and hence the copy can overflow it.
The changes to string[.h] introduce the required function.
The first commit (initially introducing it) uses some fortification checks & macros not available in 4.4 which I kept in as the get removed in the very next commit.
I also included the fixup commit moving it out of the header although I'm not sure this is strictly required in 4.4. But it doesn't hurt either.
Guenter Roeck (1):
string: uninline memcpy_and_pad
Kees Cook (1):
Bluetooth: hci_sock: Correctly bounds check and pad HCI_MON_NEW_INDEX
name
Martin Wilck (2):
string.h: add memcpy_and_pad()
string.h: un-fortify memcpy_and_pad
include/linux/string.h | 3 +++ lib/string_helpers.c | 20 ++++++++++++++++++++ net/bluetooth/hci_sock.c | 3 ++- 3 files changed, 25 insertions(+), 1 deletion(-)
Signature · 1 line
2.40.1
PATCH 1/4 string.h: add memcpy_and_pad()
From: Martin Wilck <[email protected]>
commit 01f33c336e2d298ea5d4ce5d6e5bcd12865cc30f upstream.
This helper function is useful for the nvme subsystem, and maybe others.
Note: the warnings reported by the kbuild test robot for this patch are actually generated by the use of CONFIG_PROFILE_ALL_BRANCHES together with __FORTIFY_INLINE.
--- include/linux/string.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
include/linux/string.h +29 -0
@@ -204,4 +204,33 @@ static inline const char *kbasename(const char *path) return tail ? tail + 1 : path; } +/** +* memcpy_and_pad - Copy one buffer to another with padding +* @dest: Where to copy to +* @dest_len: The destination buffer size +* @src: Where to copy from +* @count: The number of bytes to copy +* @pad: Character to use for padding if space is left in destination. +*/ +__FORTIFY_INLINE void memcpy_and_pad(void *dest, size_t dest_len, +const void *src,size_t count,int pad) +{ +size_t dest_size =__builtin_object_size (dest,0 ); +size_t src_size =__builtin_object_size (src,0 ); + +if (__builtin_constant_p (dest_len) &&__builtin_constant_p (count)) { +if (dest_size < dest_len && dest_size < count) +__write_overflow (); +else if (src_size < dest_len && src_size < count) +__read_overflow3 (); + } +if (dest_size < dest_len) +fortify_panic (__func__); +if (dest_len > count) { +memcpy (dest, src, count); +memset (dest + count, pad, dest_len - count); + }else +memcpy (dest, src, dest_len); +} +#endif /* _LINUX_STRING_H_ */
Signature · 1 line
2.40.1
PATCH 2/4 string.h: un-fortify memcpy_and_pad
In reply to Alexander GrundFrom: Martin Wilck <[email protected]>
commit 1359798f9d4082eb04575efdd19512fbd9c28464 upstream.
The way I'd implemented the new helper memcpy_and_pad with __FORTIFY_INLINE caused compiler warnings for certain kernel configurations.
This helper is only used in a single place at this time, and thus doesn't benefit much from fortification. So simplify the code by dropping fortification support for now.
--- include/linux/string.h | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
include/linux/string.h +2 -13
@@ -212,20 +212,9 @@ static inline const char *kbasename(const char *path)* @count: The number of bytes to copy * @pad: Character to use for padding if space is left in destination. */ -__FORTIFY_INLINE void memcpy_and_pad(void *dest, size_t dest_len, -const void *src,size_t count,int pad) +static inline void memcpy_and_pad (void *dest,size_t dest_len, +const void *src,size_t count,int pad) { -size_t dest_size =__builtin_object_size (dest,0 ); -size_t src_size =__builtin_object_size (src,0 ); - -if (__builtin_constant_p (dest_len) &&__builtin_constant_p (count)) { -if (dest_size < dest_len && dest_size < count) -__write_overflow (); -else if (src_size < dest_len && src_size < count) -__read_overflow3 (); - } -if (dest_size < dest_len) -fortify_panic (__func__);if (dest_len > count) {memcpy (dest, src, count);memset (dest + count, pad, dest_len - count);
Signature · 1 line
2.40.1
PATCH 3/4 string: uninline memcpy_and_pad
In reply to Alexander GrundFrom: Guenter Roeck <[email protected]>
commit 5c4e0a21fae877a7ef89be6dcc6263ec672372b8 upstream.
When building m68k:allmodconfig, recent versions of gcc generate the following error if the length of UTS_RELEASE is less than 8 bytes.
In function 'memcpy_and_pad',
inlined from 'nvmet_execute_disc_identify' at
drivers/nvme/target/discovery.c:268:2: arch/m68k/include/asm/string.h:72:25: error:
'__builtin_memcpy' reading 8 bytes from a region of size 7
Discussions around the problem suggest that this only happens if an architecture does not provide strlen(), if -ffreestanding is provided as compiler option, and if CONFIG_FORTIFY_SOURCE=n. All of this is the case for m68k. The exact reasons are unknown, but seem to be related to the ability of the compiler to evaluate the return value of strlen() and the resulting execution flow in memcpy_and_pad(). It would be possible to work around the problem by using sizeof(UTS_RELEASE) instead of strlen(UTS_RELEASE), but that would only postpone the problem until the function is called in a similar way. Uninline memcpy_and_pad() instead to solve the problem for good.
--- include/linux/string.h | 19 ++----------------- lib/string_helpers.c | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 17 deletions(-)
include/linux/string.h +2 -17
@@ -204,22 +204,7 @@ static inline const char *kbasename(const char *path) return tail ? tail + 1 : path; } -/** -* memcpy_and_pad - Copy one buffer to another with padding -* @dest: Where to copy to -* @dest_len: The destination buffer size -* @src: Where to copy from -* @count: The number of bytes to copy -* @pad: Character to use for padding if space is left in destination. -*/ -static inline void memcpy_and_pad (void *dest,size_t dest_len, -const void *src,size_t count,int pad) -{ -if (dest_len > count) { -memcpy (dest, src, count); -memset (dest + count, pad, dest_len - count); - }else -memcpy (dest, src, dest_len); -} +void memcpy_and_pad (void *dest,size_t dest_len,const void *src,size_t count, +int pad);#endif /* _LINUX_STRING_H_ */
lib/string_helpers.c +20 -0
@@ -534,3 +534,23 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,return p - dst; }EXPORT_SYMBOL (string_escape_mem); + +/** +* memcpy_and_pad - Copy one buffer to another with padding +* @dest: Where to copy to +* @dest_len: The destination buffer size +* @src: Where to copy from +* @count: The number of bytes to copy +* @pad: Character to use for padding if space is left in destination. +*/ +void memcpy_and_pad (void *dest,size_t dest_len,const void *src,size_t count, +int pad) +{ +if (dest_len > count) { +memcpy (dest, src, count); +memset (dest + count, pad, dest_len - count); + }else { +memcpy (dest, src, dest_len); + } +} +EXPORT_SYMBOL (memcpy_and_pad);
Signature · 1 line
2.40.1
PATCH 4/4 Bluetooth: hci_sock: Correctly bounds check and pad HCI_MON_NEW_INDEX name
In reply to Alexander GrundFrom: Kees Cook <[email protected]>
The code pattern of memcpy(dst, src, strlen(src)) is almost always wrong. In this case it is wrong because it leaves memory uninitialized if it is less than sizeof(ni->name), and overflows ni->name when longer.
Normally strtomem_pad() could be used here, but since ni->name is a trailing array in struct hci_mon_new_index, compilers that don't support -fstrict-flex-arrays=3 can't tell how large this array is via __builtin_object_size(). Instead, open-code the helper and use sizeof() since it will work correctly.
Additionally mark ni->name as __nonstring since it appears to not be a %NUL terminated C string.
--- net/bluetooth/hci_sock.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/bluetooth/hci_sock.c +2 -1
@@ -333,7 +333,8 @@ static struct sk_buff *create_monitor_event(struct hci_dev *hdev, int event) ni->type = hdev->dev_type; ni->bus = hdev->bus;bacpy (&ni->bdaddr, &hdev->bdaddr); -memcpy (ni->name, hdev->name,strlen (hdev->name)); +memcpy_and_pad (ni->name,sizeof (ni->name), hdev->name, +strnlen (hdev->name,sizeof (ni->name)),'\0' ); opcode =cpu_to_le16 (HCI_MON_NEW_INDEX );break ;
Signature · 1 line
2.40.1
Re: [cip-dev] [PATCH 4.4 4/4] Bluetooth: hci_sock: Correctly bounds check and pad HCI_MON_NEW_INDEX name
Hi!
Quoted 1 line
From: Kees Cook <[email protected]>
commit cb3871b1cd135a6662b732fbc6b3db4afcdb4a64 upstream.
..afaict.
Best regards,
Pavel
Signature · 2 lines
DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi!
Series looks ok to me (with missing "upstream" annotation). Uli, will you handle this?
Best regards,
Pavel
Signature · 2 lines
DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Quoted 4 lines · CET Pavel Machek
Series looks ok to me (with missing "upstream" annotation). Uli, will you handle this?
Will do.
CU
Uli
Ulrich Hecht
AppliedQuoted 6 lines · CET Alexander Grund
From: Alexander Grund <[email protected]>
This basically fixes the
memcpy(dst, src, strlen(src))where dst is a fixed size array and hence the copy can overflow it.
Thank you, applied.
CU
Uli