idr: fix invalid ptr dereference on item delete

BugLink: http://bugs.launchpad.net/bugs/1786352
commit 7a4deea1aa8bddfed4ef1b35fc2b6732563d8ad5 upstream.

If the radix tree underlying the IDR happens to be full and we attempt
to remove an id which is larger than any id in the IDR, we will call
__radix_tree_delete() with an uninitialised 'slot' pointer, at which
point anything could happen.  This was easiest to hit with a single
entry at id 0 and attempting to remove a non-0 id, but it could have
happened with 64 entries and attempting to remove an id >= 64.

Roman said:

  The syzcaller test boils down to opening /dev/kvm, creating an
  eventfd, and calling a couple of KVM ioctls. None of this requires
  superuser. And the result is dereferencing an uninitialized pointer
  which is likely a crash. The specific path caught by syzbot is via
  KVM_HYPERV_EVENTD ioctl which is new in 4.17. But I guess there are
  other user-triggerable paths, so cc:stable is probably justified.

Matthew added:

  We have around 250 calls to idr_remove() in the kernel today. Many of
  them pass an ID which is embedded in the object they're removing, so
  they're safe. Picking a few likely candidates:

  drivers/firewire/core-cdev.c looks unsafe; the ID comes from an ioctl.
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c is similar
  drivers/atm/nicstar.c could be taken down by a handcrafted packet

Link: http://lkml.kernel.org/r/20180518175025.GD6361@bombadil.infradead.org
Fixes: 0a835c4f090a ("Reimplement IDR and IDA using the radix tree")
Reported-by: <syzbot+35666cba7f0a337e2e79@syzkaller.appspotmail.com>
Debugged-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 214ed8995c3eca45f7eb203a6c3a3ace4a7b9b41..434b3270ca4556eb5303021bbaeeaab8544757e6 100644 (file)
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -2033,10 +2033,12 @@ void *radix_tree_delete_item(struct radix_tree_root *root,
                             unsigned long index, void *item)
 {
        struct radix_tree_node *node = NULL;
-       void __rcu **slot;
+       void __rcu **slot = NULL;
        void *entry;
 
        entry = __radix_tree_lookup(root, index, &node, &slot);
+       if (!slot)
+               return NULL;
        if (!entry && (!is_idr(root) || node_tag_get(root, node, IDR_FREE,
                                                get_slot_offset(node, slot))))
                return NULL;

diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 30cd0b296f1a76847122f009c2cd0f2d5b9109f4..8e61aad0ca3f7ff65ca7ef640d6f9c9c0e7b75a9 100644 (file)
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -202,6 +202,13 @@ void idr_checks(void)
        idr_remove(&idr, 3);
        idr_remove(&idr, 0);
 
+       assert(idr_alloc(&idr, DUMMY_PTR, 0, 0, GFP_KERNEL) == 0);
+       idr_remove(&idr, 1);
+       for (i = 1; i < RADIX_TREE_MAP_SIZE; i++)
+               assert(idr_alloc(&idr, DUMMY_PTR, 0, 0, GFP_KERNEL) == i);
+       idr_remove(&idr, 1 << 30);
+       idr_destroy(&idr);
+
        for (i = INT_MAX - 3UL; i < INT_MAX + 1UL; i++) {
                struct item *item = item_create(i, 0);
                assert(idr_alloc(&idr, item, i, i + 10, GFP_KERNEL) == i);