[PATCH 4.4 0/4] Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event

[PATCH 4.4 0/4] Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event

[Alexander Grund]

From: Alexander Grund <flamefire89@gmail.com>

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(-)

-- 
2.40.1

[PATCH 4.4 1/4] string.h: add memcpy_and_pad()

[Alexander Grund]

From: Martin Wilck <mwilck@suse.com>

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.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Sagi Grimberg <sagi@grimbeg.me>
Signed-off-by: Christoph Hellwig <hch@lst.de>
[AG: Backported to 4.4]
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 include/linux/string.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 1a9589a5ace62..84af888924e11 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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_ */
-- 
2.40.1

[PATCH 4.4 2/4] string.h: un-fortify memcpy_and_pad

[Alexander Grund]

From: Martin Wilck <mwilck@suse.com>

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.

Fixes: 01f33c336e2d "string.h: add memcpy_and_pad()"
Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 include/linux/string.h | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 84af888924e11..c874f5ea9bb3e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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);
-- 
2.40.1

[PATCH 4.4 3/4] string: uninline memcpy_and_pad

[Alexander Grund]

From: Guenter Roeck <linux@roeck-us.net>

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.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 include/linux/string.h | 19 ++-----------------
 lib/string_helpers.c   | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index c874f5ea9bb3e..9f745d7e9f3f7 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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_ */
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5c88204b6f1f1..f46075b3d9e41 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -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);
-- 
2.40.1

[PATCH 4.4 4/4] Bluetooth: hci_sock: Correctly bounds check and pad HCI_MON_NEW_INDEX name

[Alexander Grund]

From: Kees Cook <keescook@chromium.org>

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.

Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Edward AD <twuufnxlz@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-bluetooth@vger.kernel.org
Cc: netdev@vger.kernel.org
Fixes: 18f547f3fc07 ("Bluetooth: hci_sock: fix slab oob read in create_monitor_event")
Link: https://lore.kernel.org/lkml/202310110908.F2639D3276@keescook/
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
[AG: Remove __nonstring attribute not present in 4.4]
Signed-off-by: Alexander Grund <theflamefire89@gmail.com>
---
 net/bluetooth/hci_sock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 48fcbbde9d3f0..dbbd69bf43191 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -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;
-- 
2.40.1

Re: [cip-dev] [PATCH 4.4 4/4] Bluetooth: hci_sock: Correctly bounds check and pad HCI_MON_NEW_INDEX name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

Hi!

> From: Kees Cook <keescook@chromium.org>

commit cb3871b1cd135a6662b732fbc6b3db4afcdb4a64 upstream.

..afaict.

Best regards,
								Pavel
								
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [cip-dev] [PATCH 4.4 0/4] Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

Hi!

Series looks ok to me (with missing "upstream" annotation). Uli, will
you handle this?

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 4.4 0/4] Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event

[Ulrich Hecht]

> On 01/11/2024 8:06 PM CET Alexander Grund <theflamefire89@gmail.com> wrote:
> From: Alexander Grund <flamefire89@gmail.com>
> 
> 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

Re: [cip-dev] [PATCH 4.4 0/4] Bluetooth: hci_sock: Fix possible OOB write in create_monitor_event

[Ulrich Hecht]

> On 01/15/2024 9:05 PM CET Pavel Machek <pavel@denx.de> wrote:
> Series looks ok to me (with missing "upstream" annotation). Uli, will
> you handle this?

Will do.

CU
Uli