lkml.org 
[lkml]   [2016]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 010/305] Bluetooth: vhci: fix open_timeout vs. hdev race
    3.16.37-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Jiri Slaby <jslaby@suse.cz>

    commit 373a32c848ae3a1c03618517cce85f9211a6facf upstream.

    Both vhci_get_user and vhci_release race with open_timeout work. They
    both contain cancel_delayed_work_sync, but do not test whether the
    work actually created hdev or not. Since the work can be in progress
    and _sync will wait for finishing it, we can have data->hdev allocated
    when cancel_delayed_work_sync returns. But the call sites do 'if
    (data->hdev)' *before* cancel_delayed_work_sync.

    As a result:
    * vhci_get_user allocates a second hdev and puts it into
    data->hdev. The former is leaked.
    * vhci_release does not release data->hdev properly as it thinks there
    is none.

    Fix both cases by moving the actual test *after* the call to
    cancel_delayed_work_sync.

    This can be hit by this program:
    #include <err.h>
    #include <fcntl.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <time.h>
    #include <unistd.h>

    #include <sys/stat.h>
    #include <sys/types.h>

    int main(int argc, char **argv)
    {
    int fd;

    srand(time(NULL));

    while (1) {
    const int delta = (rand() % 200 - 100) * 100;

    fd = open("/dev/vhci", O_RDWR);
    if (fd < 0)
    err(1, "open");

    usleep(1000000 + delta);

    close(fd);
    }

    return 0;
    }

    And the result is:
    BUG: KASAN: use-after-free in skb_queue_tail+0x13e/0x150 at addr ffff88006b0c1228
    Read of size 8 by task kworker/u13:1/32068
    =============================================================================
    BUG kmalloc-192 (Tainted: G E ): kasan: bad access detected
    -----------------------------------------------------------------------------

    Disabling lock debugging due to kernel taint
    INFO: Allocated in vhci_open+0x50/0x330 [hci_vhci] age=260 cpu=3 pid=32040
    ...
    kmem_cache_alloc_trace+0x150/0x190
    vhci_open+0x50/0x330 [hci_vhci]
    misc_open+0x35b/0x4e0
    chrdev_open+0x23b/0x510
    ...
    INFO: Freed in vhci_release+0xa4/0xd0 [hci_vhci] age=9 cpu=2 pid=32040
    ...
    __slab_free+0x204/0x310
    vhci_release+0xa4/0xd0 [hci_vhci]
    ...
    INFO: Slab 0xffffea0001ac3000 objects=16 used=13 fp=0xffff88006b0c1e00 flags=0x5fffff80004080
    INFO: Object 0xffff88006b0c1200 @offset=4608 fp=0xffff88006b0c0600
    Bytes b4 ffff88006b0c11f0: 09 df 00 00 01 00 00 00 00 00 00 00 00 00 00 00 ................
    Object ffff88006b0c1200: 00 06 0c 6b 00 88 ff ff 00 00 00 00 00 00 00 00 ...k............
    Object ffff88006b0c1210: 10 12 0c 6b 00 88 ff ff 10 12 0c 6b 00 88 ff ff ...k.......k....
    Object ffff88006b0c1220: c0 46 c2 6b 00 88 ff ff c0 46 c2 6b 00 88 ff ff .F.k.....F.k....
    Object ffff88006b0c1230: 01 00 00 00 01 00 00 00 e0 ff ff ff 0f 00 00 00 ................
    Object ffff88006b0c1240: 40 12 0c 6b 00 88 ff ff 40 12 0c 6b 00 88 ff ff @..k....@..k....
    Object ffff88006b0c1250: 50 0d 6e a0 ff ff ff ff 00 02 00 00 00 00 ad de P.n.............
    Object ffff88006b0c1260: 00 00 00 00 00 00 00 00 ab 62 02 00 01 00 00 00 .........b......
    Object ffff88006b0c1270: 90 b9 19 81 ff ff ff ff 38 12 0c 6b 00 88 ff ff ........8..k....
    Object ffff88006b0c1280: 03 00 20 00 ff ff ff ff ff ff ff ff 00 00 00 00 .. .............
    Object ffff88006b0c1290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
    Object ffff88006b0c12a0: 00 00 00 00 00 00 00 00 00 80 cd 3d 00 88 ff ff ...........=....
    Object ffff88006b0c12b0: 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 . ..............
    Redzone ffff88006b0c12c0: bb bb bb bb bb bb bb bb ........
    Padding ffff88006b0c13f8: 00 00 00 00 00 00 00 00 ........
    CPU: 3 PID: 32068 Comm: kworker/u13:1 Tainted: G B E 4.4.6-0-default #1
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20151112_172657-sheep25 04/01/2014
    Workqueue: hci0 hci_cmd_work [bluetooth]
    00000000ffffffff ffffffff81926cfa ffff88006be37c68 ffff88006bc27180
    ffff88006b0c1200 ffff88006b0c1234 ffffffff81577993 ffffffff82489320
    ffff88006bc24240 0000000000000046 ffff88006a100000 000000026e51eb80
    Call Trace:
    ...
    [<ffffffff81ec8ebe>] ? skb_queue_tail+0x13e/0x150
    [<ffffffffa06e027c>] ? vhci_send_frame+0xac/0x100 [hci_vhci]
    [<ffffffffa0c61268>] ? hci_send_frame+0x188/0x320 [bluetooth]
    [<ffffffffa0c61515>] ? hci_cmd_work+0x115/0x310 [bluetooth]
    [<ffffffff811a1375>] ? process_one_work+0x815/0x1340
    [<ffffffff811a1f85>] ? worker_thread+0xe5/0x11f0
    [<ffffffff811a1ea0>] ? process_one_work+0x1340/0x1340
    [<ffffffff811b3c68>] ? kthread+0x1c8/0x230
    ...
    Memory state around the buggy address:
    ffff88006b0c1100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
    ffff88006b0c1180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
    >ffff88006b0c1200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
    ^
    ffff88006b0c1280: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
    ffff88006b0c1300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

    Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
    Signed-off-by: Jiri Slaby <jslaby@suse.cz>
    Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
    Cc: Dmitry Vyukov <dvyukov@google.com>
    [bwh: Backported to 3.16: adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    drivers/bluetooth/hci_vhci.c | 8 +++++---
    1 file changed, 5 insertions(+), 3 deletions(-)

    --- a/drivers/bluetooth/hci_vhci.c
    +++ b/drivers/bluetooth/hci_vhci.c
    @@ -183,13 +183,13 @@ static inline ssize_t vhci_get_user(stru
    break;

    case HCI_VENDOR_PKT:
    + cancel_delayed_work_sync(&data->open_timeout);
    +
    if (data->hdev) {
    kfree_skb(skb);
    return -EBADFD;
    }

    - cancel_delayed_work_sync(&data->open_timeout);
    -
    dev_type = *((__u8 *) skb->data);
    skb_pull(skb, 1);

    @@ -331,10 +331,12 @@ static int vhci_open(struct inode *inode
    static int vhci_release(struct inode *inode, struct file *file)
    {
    struct vhci_data *data = file->private_data;
    - struct hci_dev *hdev = data->hdev;
    + struct hci_dev *hdev;

    cancel_delayed_work_sync(&data->open_timeout);

    + hdev = data->hdev;
    +
    if (hdev) {
    hci_unregister_dev(hdev);
    hci_free_dev(hdev);
    \
     
     \ /
      Last update: 2016-09-17 09:56    [W:4.093 / U:1.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site