| Message ID | 20231010053656.2034368-2-twuufnxlz@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bluetooth: hci_sock: fix slab oob read in create_monitor_event | expand |
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Tue, 10 Oct 2023 13:36:57 +0800 you wrote: > When accessing hdev->name, the actual string length should prevail > > Reported-by: syzbot+c90849c50ed209d77689@syzkaller.appspotmail.com > Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings") > Signed-off-by: Edward AD <twuufnxlz@gmail.com> > --- > net/bluetooth/hci_sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Here is the summary with links: - Bluetooth: hci_sock: fix slab oob read in create_monitor_event https://git.kernel.org/bluetooth/bluetooth-next/c/78480de55a96 You are awesome, thank you!
On Tue, Oct 10, 2023 at 01:36:57PM +0800, Edward AD wrote: > When accessing hdev->name, the actual string length should prevail > > Reported-by: syzbot+c90849c50ed209d77689@syzkaller.appspotmail.com > Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings") > Signed-off-by: Edward AD <twuufnxlz@gmail.com> > --- > net/bluetooth/hci_sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c > index 5e4f718073b7..72abe54c45dd 100644 > --- a/net/bluetooth/hci_sock.c > +++ b/net/bluetooth/hci_sock.c > @@ -488,7 +488,7 @@ 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, 8); > + memcpy(ni->name, hdev->name, strlen(hdev->name)); Uh, what's going on here? hdev is: struct hci_dev { ... const char *name; ni is: struct hci_mon_new_index { char name[8]; You can't use "strlen" here in the case that "hdev->name" is larger than 8 bytes. Also, why memcpy() and not strscpy()? Is this supposed to be padded out with %NUL bytes? It appears to be sent over the network, so "yes" seems to be the safe answer. Should ni->name be always %NUL terminated? That I can't tell for sure, but I assume "no", because the solution was to explicitly copy all the bytes _except_ the %NUL byte (using strlen). struct hci_mon_new_index's "name" should be marked __nonstring, and instead strtomem_pad() should be used instead of memcpy. -Kees > > opcode = cpu_to_le16(HCI_MON_NEW_INDEX); > break; > -- > 2.25.1 >
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index 5e4f718073b7..72abe54c45dd 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -488,7 +488,7 @@ 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, 8); + memcpy(ni->name, hdev->name, strlen(hdev->name)); opcode = cpu_to_le16(HCI_MON_NEW_INDEX); break;
When accessing hdev->name, the actual string length should prevail Reported-by: syzbot+c90849c50ed209d77689@syzkaller.appspotmail.com Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings") Signed-off-by: Edward AD <twuufnxlz@gmail.com> --- net/bluetooth/hci_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)