In commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute
mismatch") we switched from always calling remap_pfn_range() to call
dma_mmap_coherent() to handle issues with systems with non-coherent USB host
controller drivers. Unfortunatly, as syzbot quickly told us, not all the world
is host controllers with DMA support, so we need to check what host controller
we are attempting to talk to before doing this type of allocation.
Thanks to Christoph for the quick idea of how to fix this.
Cc: Christoph Hellwig <hch(a)lst.de>
Cc: Hillf Danton <hdanton(a)sina.com>
Cc: Thomas Gleixner <tglx(a)linutronix.de>
Cc: Jeremy Linton <jeremy.linton(a)arm.com>
Reported-by: syzbot+353be47c9ce21b68b7ed(a)syzkaller.appspotmail.com
Fixes: 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch")
Cc: stable <stable(a)vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/usb/core/devio.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index b9db9812d6c5..d93d94d7ff50 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -251,9 +251,19 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
usbm->vma_use_count = 1;
INIT_LIST_HEAD(&usbm->memlist);
- if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle, size)) {
- dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
- return -EAGAIN;
+ if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
+ if (remap_pfn_range(vma, vma->vm_start,
+ virt_to_phys(usbm->mem) >> PAGE_SHIFT,
+ size, vma->vm_page_prot) < 0) {
+ dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+ return -EAGAIN;
+ }
+ } else {
+ if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
+ size)) {
+ dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+ return -EAGAIN;
+ }
}
vma->vm_flags |= VM_IO;
--
2.26.2
The following race occurs while accessing the dmabuf object exported as
file:
P1 P2
dma_buf_release() dmabuffs_dname()
[say lsof reading /proc/<P1 pid>/fd/<num>]
read dmabuf stored in dentry->d_fsdata
Free the dmabuf object
Start accessing the dmabuf structure
In the above description, the dmabuf object freed in P1 is being
accessed from P2 which is resulting into the use-after-free. Below is
the dump stack reported.
We are reading the dmabuf object stored in the dentry->d_fsdata but
there is no binding between the dentry and the dmabuf which means that
the dmabuf can be freed while it is being read from ->d_fsdata and
inuse. Reviews on the patch V1 says that protecting the dmabuf inuse
with an extra refcount is not a viable solution as the exported dmabuf
is already under file's refcount and keeping the multiple refcounts on
the same object coordinated is not possible.
As we are reading the dmabuf in ->d_fsdata just to get the user passed
name, we can directly store the name in d_fsdata thus can avoid the
reading of dmabuf altogether.
Call Trace:
kasan_report+0x12/0x20
__asan_report_load8_noabort+0x14/0x20
dmabuffs_dname+0x4f4/0x560
tomoyo_realpath_from_path+0x165/0x660
tomoyo_get_realpath
tomoyo_check_open_permission+0x2a3/0x3e0
tomoyo_file_open
tomoyo_file_open+0xa9/0xd0
security_file_open+0x71/0x300
do_dentry_open+0x37a/0x1380
vfs_open+0xa0/0xd0
path_openat+0x12ee/0x3490
do_filp_open+0x192/0x260
do_sys_openat2+0x5eb/0x7e0
do_sys_open+0xf2/0x180
Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
Reported-by: syzbot+3643a18836bce555bff6(a)syzkaller.appspotmail.com
Cc: <stable(a)vger.kernel.org> [5.3+]
Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org>
---
Changes in v2:
- Pass the user passed name in ->d_fsdata instead of dmabuf
- Improve the commit message
Changes in v1: (https://patchwork.kernel.org/patch/11514063/)
drivers/dma-buf/dma-buf.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 01ce125..0071f7d 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -25,6 +25,7 @@
#include <linux/mm.h>
#include <linux/mount.h>
#include <linux/pseudo_fs.h>
+#include <linux/dcache.h>
#include <uapi/linux/dma-buf.h>
#include <uapi/linux/magic.h>
@@ -40,15 +41,13 @@ struct dma_buf_list {
static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
{
- struct dma_buf *dmabuf;
char name[DMA_BUF_NAME_LEN];
size_t ret = 0;
- dmabuf = dentry->d_fsdata;
- dma_resv_lock(dmabuf->resv, NULL);
- if (dmabuf->name)
- ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
- dma_resv_unlock(dmabuf->resv);
+ spin_lock(&dentry->d_lock);
+ if (dentry->d_fsdata)
+ ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN);
+ spin_unlock(&dentry->d_lock);
return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
dentry->d_name.name, ret > 0 ? name : "");
@@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc)
static int dma_buf_release(struct inode *inode, struct file *file)
{
struct dma_buf *dmabuf;
+ struct dentry *dentry = file->f_path.dentry;
if (!is_dma_buf_file(file))
return -EINVAL;
dmabuf = file->private_data;
+ spin_lock(&dentry->d_lock);
+ dentry->d_fsdata = NULL;
+ spin_unlock(&dentry->d_lock);
BUG_ON(dmabuf->vmapping_counter);
/*
@@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
}
kfree(dmabuf->name);
dmabuf->name = name;
+ dmabuf->file->f_path.dentry->d_fsdata = name;
out_unlock:
dma_resv_unlock(dmabuf->resv);
@@ -446,7 +450,6 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
goto err_alloc_file;
file->f_flags = flags & (O_ACCMODE | O_NONBLOCK);
file->private_data = dmabuf;
- file->f_path.dentry->d_fsdata = dmabuf;
return file;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
pull in arbitrary other resources, including CRTCs (e.g. when
reconfiguring global resources).
But in nonblocking mode userspace has then no idea this happened,
which can lead to spurious EBUSY calls, both:
- when that other CRTC is currently busy doing a page_flip the
ALLOW_MODESET commit can fail with an EBUSY
- on the other CRTC a normal atomic flip can fail with EBUSY because
of the additional commit inserted by the kernel without userspace's
knowledge
For blocking commits this isn't a problem, because everyone else will
just block until all the CRTC are reconfigured. Only thing userspace
can notice is the dropped frames without any reason for why frames got
dropped.
Consensus is that we need new uapi to handle this properly, but no one
has any idea what exactly the new uapi should look like. As a stop-gap
plug this problem by demoting nonblocking commits which might cause
issues by including CRTCs not in the original request to blocking
commits.
v2: Add comments and a WARN_ON to enforce this only when allowed - we
don't want to silently convert page flips into blocking plane updates
just because the driver is buggy.
v3: Fix inverted WARN_ON (Pekka).
References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568
Cc: Daniel Stone <daniel(a)fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen(a)collabora.co.uk>
Cc: stable(a)vger.kernel.org
Reviewed-by: Daniel Stone <daniels(a)collabora.com>
Cc: Ville Syrjälä <ville.syrjala(a)linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
--
Resending because last attempt failed CI and meanwhile the results are
lost :-/
-Daniel
---
drivers/gpu/drm/drm_atomic.c | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 965173fd0ac2..4f140ff6fb98 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1362,15 +1362,43 @@ EXPORT_SYMBOL(drm_atomic_commit);
int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
{
struct drm_mode_config *config = &state->dev->mode_config;
- int ret;
+ unsigned requested_crtc = 0;
+ unsigned affected_crtc = 0;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ bool nonblocking = true;
+ int ret, i;
+
+ /*
+ * For commits that allow modesets drivers can add other CRTCs to the
+ * atomic commit, e.g. when they need to reallocate global resources.
+ *
+ * But when userspace also requests a nonblocking commit then userspace
+ * cannot know that the commit affects other CRTCs, which can result in
+ * spurious EBUSY failures. Until we have better uapi plug this by
+ * demoting such commits to blocking mode.
+ */
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+ requested_crtc |= drm_crtc_mask(crtc);
ret = drm_atomic_check_only(state);
if (ret)
return ret;
- DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+ affected_crtc |= drm_crtc_mask(crtc);
+
+ if (affected_crtc != requested_crtc) {
+ /* adding other CRTC is only allowed for modeset commits */
+ WARN_ON(!state->allow_modeset);
+
+ DRM_DEBUG_ATOMIC("demoting %p to blocking mode to avoid EBUSY\n", state);
+ nonblocking = false;
+ } else {
+ DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+ }
- return config->funcs->atomic_commit(state->dev, state, true);
+ return config->funcs->atomic_commit(state->dev, state, nonblocking);
}
EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
--
2.25.1
From: Sriharsha Allenki <sallenki(a)codeaurora.org>
On platforms with IOMMU enabled, multiple SGs can be coalesced into one
by the IOMMU driver. In that case the SG list processing as part of the
completion of a urb on a bulk endpoint can result into a NULL pointer
dereference with the below stack dump.
<6> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
<6> pgd = c0004000
<6> [0000000c] *pgd=00000000
<6> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
<2> PC is at xhci_queue_bulk_tx+0x454/0x80c
<2> LR is at xhci_queue_bulk_tx+0x44c/0x80c
<2> pc : [<c08907c4>] lr : [<c08907bc>] psr: 000000d3
<2> sp : ca337c80 ip : 00000000 fp : ffffffff
<2> r10: 00000000 r9 : 50037000 r8 : 00004000
<2> r7 : 00000000 r6 : 00004000 r5 : 00000000 r4 : 00000000
<2> r3 : 00000000 r2 : 00000082 r1 : c2c1a200 r0 : 00000000
<2> Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
<2> Control: 10c0383d Table: b412c06a DAC: 00000051
<6> Process usb-storage (pid: 5961, stack limit = 0xca336210)
<snip>
<2> [<c08907c4>] (xhci_queue_bulk_tx)
<2> [<c0881b3c>] (xhci_urb_enqueue)
<2> [<c0831068>] (usb_hcd_submit_urb)
<2> [<c08350b4>] (usb_sg_wait)
<2> [<c089f384>] (usb_stor_bulk_transfer_sglist)
<2> [<c089f2c0>] (usb_stor_bulk_srb)
<2> [<c089fe38>] (usb_stor_Bulk_transport)
<2> [<c089f468>] (usb_stor_invoke_transport)
<2> [<c08a11b4>] (usb_stor_control_thread)
<2> [<c014a534>] (kthread)
The above NULL pointer dereference is the result of block_len and the
sent_len set to zero after the first SG of the list when IOMMU driver
is enabled. Because of this the loop of processing the SGs has run
more than num_sgs which resulted in a sg_next on the last SG of the
list which has SG_END set.
Fix this by check for the sg before any attributes of the sg are
accessed.
[modified reason for null pointer dereference in commit message subject -Mathias]
Fixes: f9c589e142d04 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: stable(a)vger.kernel.org
Signed-off-by: Sriharsha Allenki <sallenki(a)codeaurora.org>
Signed-off-by: Mathias Nyman <mathias.nyman(a)linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0fda0c0f4d31..2c255d0620b0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3433,8 +3433,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
/* New sg entry */
--num_sgs;
sent_len -= block_len;
- if (num_sgs != 0) {
- sg = sg_next(sg);
+ sg = sg_next(sg);
+ if (num_sgs != 0 && sg) {
block_len = sg_dma_len(sg);
addr = (u64) sg_dma_address(sg);
addr += sent_len;
--
2.17.1