This is a note to let you know that I've just added the patch titled
USB: w1 ds2490: Fix bug caused by improper use of altsetting array
to my usb git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-linus branch.
The patch will show up in the next release of the linux-next tree
(usually sometime within the next 24 hours during the week.)
The patch will hopefully also be merged in Linus's tree for the
next -rc kernel release.
If you have any questions about this process, please let me know.
>From c114944d7d67f24e71562fcfc18d550ab787e4d4 Mon Sep 17 00:00:00 2001
From: Alan Stern <stern(a)rowland.harvard.edu>
Date: Mon, 22 Apr 2019 11:16:04 -0400
Subject: USB: w1 ds2490: Fix bug caused by improper use of altsetting array
The syzkaller USB fuzzer spotted a slab-out-of-bounds bug in the
ds2490 driver. This bug is caused by improper use of the altsetting
array in the usb_interface structure (the array's entries are not
always stored in numerical order), combined with a naive assumption
that all interfaces probed by the driver will have the expected number
of altsettings.
The bug can be fixed by replacing references to the possibly
non-existent intf->altsetting[alt] entry with the guaranteed-to-exist
intf->cur_altsetting entry.
Signed-off-by: Alan Stern <stern(a)rowland.harvard.edu>
Reported-and-tested-by: syzbot+d65f673b847a1a96cdba(a)syzkaller.appspotmail.com
CC: <stable(a)vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/w1/masters/ds2490.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 0f4ecfcdb549..a9fb77585272 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -1016,15 +1016,15 @@ static int ds_probe(struct usb_interface *intf,
/* alternative 3, 1ms interrupt (greatly speeds search), 64 byte bulk */
alt = 3;
err = usb_set_interface(dev->udev,
- intf->altsetting[alt].desc.bInterfaceNumber, alt);
+ intf->cur_altsetting->desc.bInterfaceNumber, alt);
if (err) {
dev_err(&dev->udev->dev, "Failed to set alternative setting %d "
"for %d interface: err=%d.\n", alt,
- intf->altsetting[alt].desc.bInterfaceNumber, err);
+ intf->cur_altsetting->desc.bInterfaceNumber, err);
goto err_out_clear;
}
- iface_desc = &intf->altsetting[alt];
+ iface_desc = intf->cur_altsetting;
if (iface_desc->desc.bNumEndpoints != NUM_EP-1) {
pr_info("Num endpoints=%d. It is not DS9490R.\n",
iface_desc->desc.bNumEndpoints);
--
2.21.0
This is a note to let you know that I've just added the patch titled
usb: usbip: fix isoc packet num validation in get_pipe
to my usb git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-linus branch.
The patch will show up in the next release of the linux-next tree
(usually sometime within the next 24 hours during the week.)
The patch will hopefully also be merged in Linus's tree for the
next -rc kernel release.
If you have any questions about this process, please let me know.
>From c409ca3be3c6ff3a1eeb303b191184e80d412862 Mon Sep 17 00:00:00 2001
From: Malte Leip <malte(a)leip.net>
Date: Sun, 14 Apr 2019 12:00:12 +0200
Subject: usb: usbip: fix isoc packet num validation in get_pipe
Change the validation of number_of_packets in get_pipe to compare the
number of packets to a fixed maximum number of packets allowed, set to
be 1024. This number was chosen due to it being used by other drivers as
well, for example drivers/usb/host/uhci-q.c
Background/reason:
The get_pipe function in stub_rx.c validates the number of packets in
isochronous mode and aborts with an error if that number is too large,
in order to prevent malicious input from possibly triggering large
memory allocations. This was previously done by checking whether
pdu->u.cmd_submit.number_of_packets is bigger than the number of packets
that would be needed for pdu->u.cmd_submit.transfer_buffer_length bytes
if all except possibly the last packet had maximum length, given by
usb_endpoint_maxp(epd) * usb_endpoint_maxp_mult(epd). This leads to an
error if URBs with packets shorter than the maximum possible length are
submitted, which is allowed according to
Documentation/driver-api/usb/URB.rst and occurs for example with the
snd-usb-audio driver.
Fixes: c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input")
Signed-off-by: Malte Leip <malte(a)leip.net>
Cc: stable <stable(a)vger.kernel.org>
Acked-by: Shuah Khan <skhan(a)linuxfoundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/usb/usbip/stub_rx.c | 12 +++---------
drivers/usb/usbip/usbip_common.h | 7 +++++++
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 97b09a42a10c..dbfb2f24d71e 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -361,16 +361,10 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
}
if (usb_endpoint_xfer_isoc(epd)) {
- /* validate packet size and number of packets */
- unsigned int maxp, packets, bytes;
-
- maxp = usb_endpoint_maxp(epd);
- maxp *= usb_endpoint_maxp_mult(epd);
- bytes = pdu->u.cmd_submit.transfer_buffer_length;
- packets = DIV_ROUND_UP(bytes, maxp);
-
+ /* validate number of packets */
if (pdu->u.cmd_submit.number_of_packets < 0 ||
- pdu->u.cmd_submit.number_of_packets > packets) {
+ pdu->u.cmd_submit.number_of_packets >
+ USBIP_MAX_ISO_PACKETS) {
dev_err(&sdev->udev->dev,
"CMD_SUBMIT: isoc invalid num packets %d\n",
pdu->u.cmd_submit.number_of_packets);
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index bf8afe9b5883..8be857a4fa13 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -121,6 +121,13 @@ extern struct device_attribute dev_attr_usbip_debug;
#define USBIP_DIR_OUT 0x00
#define USBIP_DIR_IN 0x01
+/*
+ * Arbitrary limit for the maximum number of isochronous packets in an URB,
+ * compare for example the uhci_submit_isochronous function in
+ * drivers/usb/host/uhci-q.c
+ */
+#define USBIP_MAX_ISO_PACKETS 1024
+
/**
* struct usbip_header_basic - data pertinent to every request
* @command: the usbip request type
--
2.21.0
On 25/04/2019 10:53, Jan Beulich wrote:
>>>> On 23.04.19 at 12:54, <jgross(a)suse.com> wrote:
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>> /* Convert the size to actually allocated. */
>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>
>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>> - range_straddles_page_boundary(phys, size))
>> + if ((dev_addr + size - 1 <= dma_mask) &&
>> + !WARN_ON(range_straddles_page_boundary(phys, size)))
>> xen_destroy_contiguous_region(phys, order);
>
> On the allocation side we have
>
> if (((dev_addr + size - 1 <= dma_mask)) &&
> !range_straddles_page_boundary(phys, size))
> *dma_handle = dev_addr;
> else {
> if (xen_create_contiguous_region(phys, order,
> fls64(dma_mask), dma_handle) != 0) {
> xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
> return NULL;
> }
> }
>
> which is (as far as the function call is concerned)
>
> if ((dev_addr + size - 1 > dma_mask) ||
> range_straddles_page_boundary(phys, size))
> xen_create_contiguous_region(...);
>
> So I don't think your transformation is correct.
I know that I modified the condition. Calling
xen_destroy_contiguous_region() for a non-contiguous region is
destructive, so we have to avoid it in any case.
I could be talked into moving both tests into the WARN_ON(), however.
Juergen
This is a note to let you know that I've just added the patch titled
usb: dwc3: Allow building USB_DWC3_QCOM without EXTCON
to my usb git tree which can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-testing branch.
The patch will show up in the next release of the linux-next tree
(usually sometime within the next 24 hours during the week.)
The patch will be merged to the usb-next branch sometime soon,
after it passes testing, and the merge window is open.
If you have any questions about this process, please let me know.
>From 77a4946516fe488b6a33390de6d749f934a243ba Mon Sep 17 00:00:00 2001
From: Marc Gonzalez <marc.w.gonzalez(a)free.fr>
Date: Wed, 24 Apr 2019 17:00:57 +0200
Subject: usb: dwc3: Allow building USB_DWC3_QCOM without EXTCON
Keep EXTCON support optional, as some platforms do not need it.
Do the same for USB_DWC3_OMAP while we're at it.
Fixes: 3def4031b3e3f ("usb: dwc3: add EXTCON dependency for qcom")
Signed-off-by: Marc Gonzalez <marc.w.gonzalez(a)free.fr>
Cc: stable <stable(a)vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/usb/dwc3/Kconfig | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 2b1494460d0c..784309435916 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -54,7 +54,8 @@ comment "Platform Glue Driver Support"
config USB_DWC3_OMAP
tristate "Texas Instruments OMAP5 and similar Platforms"
- depends on EXTCON && (ARCH_OMAP2PLUS || COMPILE_TEST)
+ depends on ARCH_OMAP2PLUS || COMPILE_TEST
+ depends on EXTCON || !EXTCON
depends on OF
default USB_DWC3
help
@@ -115,7 +116,8 @@ config USB_DWC3_ST
config USB_DWC3_QCOM
tristate "Qualcomm Platform"
- depends on EXTCON && (ARCH_QCOM || COMPILE_TEST)
+ depends on ARCH_QCOM || COMPILE_TEST
+ depends on EXTCON || !EXTCON
depends on OF
default USB_DWC3
help
--
2.21.0
On 25/04/2019 10:53, Jan Beulich wrote:
>>>> On 23.04.19 at 12:54, <jgross(a)suse.com> wrote:
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
>> /* Convert the size to actually allocated. */
>> size = 1UL << (order + XEN_PAGE_SHIFT);
>>
>> - if (((dev_addr + size - 1 <= dma_mask)) ||
>> - range_straddles_page_boundary(phys, size))
>> + if ((dev_addr + size - 1 <= dma_mask) &&
>> + !WARN_ON(range_straddles_page_boundary(phys, size)))
>> xen_destroy_contiguous_region(phys, order);
>
> On the allocation side we have
>
> if (((dev_addr + size - 1 <= dma_mask)) &&
> !range_straddles_page_boundary(phys, size))
> *dma_handle = dev_addr;
> else {
> if (xen_create_contiguous_region(phys, order,
> fls64(dma_mask), dma_handle) != 0) {
> xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs);
> return NULL;
> }
> }
>
> which is (as far as the function call is concerned)
>
> if ((dev_addr + size - 1 > dma_mask) ||
> range_straddles_page_boundary(phys, size))
> xen_create_contiguous_region(...);
>
> So I don't think your transformation is correct. Even worse, both
> parts of the condition in xen_swiotlb_free_coherent() act on an
> address that is the _result_ of the prior
> xen_create_contiguous_region(), i.e. the address should always
> match _both_ criteria anyway. Whereas what you really want is
> undo the xen_create_contiguous_region() only when it actually
> was called. Otherwise you also shatter contiguous allocations
> that were contiguous already for other reasons (perhaps just
> luck).
Yes, that is what patch 3 does.
Juergen
The condition in xen_swiotlb_free_coherent() for deciding whether to
call xen_destroy_contiguous_region() is wrong: in case the region to
be freed is not contiguous calling xen_destroy_contiguous_region() is
the wrong thing to do: it would result in inconsistent mappings of
multiple PFNs to the same MFN. This will lead to various strange
crashes or data corruption.
Instead of calling xen_destroy_contiguous_region() in that case a
warning should be issued as that situation should never occur.
Cc: stable(a)vger.kernel.org
Signed-off-by: Juergen Gross <jgross(a)suse.com>
---
drivers/xen/swiotlb-xen.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 877baf2a94f4..42a3924e6d91 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
- if (((dev_addr + size - 1 <= dma_mask)) ||
- range_straddles_page_boundary(phys, size))
+ if ((dev_addr + size - 1 <= dma_mask) &&
+ !WARN_ON(range_straddles_page_boundary(phys, size)))
xen_destroy_contiguous_region(phys, order);
xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
--
2.16.4
From: Tycho Andersen <tycho(a)tycho.ws>
As the comment notes, the return codes for TSYNC and NEW_LISTENER
conflict, because they both return positive values, one in the case of
success and one in the case of error. So, let's disallow both of these
flags together.
While this is technically a userspace break, all the users I know
of are still waiting on me to land this feature in libseccomp, so I
think it'll be safe. Also, at present my use case doesn't require
TSYNC at all, so this isn't a big deal to disallow. If someone
wanted to support this, a path forward would be to add a new flag like
TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN,
but the use cases are so different I don't see it really happening.
Finally, it's worth noting that this does actually fix a UAF issue: at the
end of seccomp_set_mode_filter(), we have:
if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
if (ret < 0) {
listener_f->private_data = NULL;
fput(listener_f);
put_unused_fd(listener);
} else {
fd_install(listener, listener_f);
ret = listener;
}
}
out_free:
seccomp_filter_free(prepared);
But if ret > 0 because TSYNC raced, we'll install the listener fd and then
free the filter out from underneath it, causing a UAF when the task closes
it or dies. This patch also switches the condition to be simply if (ret),
so that if someone does add the flag mentioned above, they won't have to
remember to fix this too.
Reported-by: syzbot+b562969adb2e04af3442(a)syzkaller.appspotmail.com
Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
CC: stable(a)vger.kernel.org # v5.0+
Signed-off-by: Tycho Andersen <tycho(a)tycho.ws>
Signed-off-by: Kees Cook <keescook(a)chromium.org>
---
kernel/seccomp.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index df27e499956a..3582eeb59893 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -502,7 +502,10 @@ seccomp_prepare_user_filter(const char __user *user_filter)
*
* Caller must be holding current->sighand->siglock lock.
*
- * Returns 0 on success, -ve on error.
+ * Returns 0 on success, -ve on error, or
+ * - in TSYNC mode: the pid of a thread which was either not in the correct
+ * seccomp mode or did not have an ancestral seccomp filter
+ * - in NEW_LISTENER mode: the fd of the new listener
*/
static long seccomp_attach_filter(unsigned int flags,
struct seccomp_filter *filter)
@@ -1258,6 +1261,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
if (flags & ~SECCOMP_FILTER_FLAG_MASK)
return -EINVAL;
+ /*
+ * In the successful case, NEW_LISTENER returns the new listener fd.
+ * But in the failure case, TSYNC returns the thread that died. If you
+ * combine these two flags, there's no way to tell whether something
+ * succeeded or failed. So, let's disallow this combination.
+ */
+ if ((flags & SECCOMP_FILTER_FLAG_TSYNC) &&
+ (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER))
+ return -EINVAL;
+
/* Prepare the new filter before holding any locks. */
prepared = seccomp_prepare_user_filter(filter);
if (IS_ERR(prepared))
@@ -1304,7 +1317,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
mutex_unlock(¤t->signal->cred_guard_mutex);
out_put_fd:
if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
- if (ret < 0) {
+ if (ret) {
listener_f->private_data = NULL;
fput(listener_f);
put_unused_fd(listener);
--
2.17.1
Some seccomp flags will become exclusive, so the selftest needs to
be adjusted to mask those out and test them individually for the "all
flags" tests.
Cc: stable(a)vger.kernel.org # v5.0+
Signed-off-by: Kees Cook <keescook(a)chromium.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++-----
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index f69d2ee29742..5019cdae5d0b 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -2166,11 +2166,14 @@ TEST(detect_seccomp_filter_flags)
SECCOMP_FILTER_FLAG_LOG,
SECCOMP_FILTER_FLAG_SPEC_ALLOW,
SECCOMP_FILTER_FLAG_NEW_LISTENER };
- unsigned int flag, all_flags;
+ unsigned int exclusive[] = {
+ SECCOMP_FILTER_FLAG_TSYNC,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER };
+ unsigned int flag, all_flags, exclusive_mask;
int i;
long ret;
- /* Test detection of known-good filter flags */
+ /* Test detection of individual known-good filter flags */
for (i = 0, all_flags = 0; i < ARRAY_SIZE(flags); i++) {
int bits = 0;
@@ -2197,16 +2200,29 @@ TEST(detect_seccomp_filter_flags)
all_flags |= flag;
}
- /* Test detection of all known-good filter flags */
- ret = seccomp(SECCOMP_SET_MODE_FILTER, all_flags, NULL);
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(EFAULT, errno) {
- TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
- all_flags);
+ /*
+ * Test detection of all known-good filter flags combined. But
+ * for the exclusive flags we need to mask them out and try them
+ * individually for the "all flags" testing.
+ */
+ exclusive_mask = 0;
+ for (i = 0; i < ARRAY_SIZE(exclusive); i++)
+ exclusive_mask |= exclusive[i];
+ for (i = 0; i < ARRAY_SIZE(exclusive); i++) {
+ flag = all_flags & ~exclusive_mask;
+ flag |= exclusive[i];
+
+ ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EFAULT, errno) {
+ TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
+ flag);
+ }
}
- /* Test detection of an unknown filter flag */
+ /* Test detection of an unknown filter flags, without exclusives. */
flag = -1;
+ flag &= ~exclusive_mask;
ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
EXPECT_EQ(-1, ret);
EXPECT_EQ(EINVAL, errno) {
--
2.17.1