linux-kernel - Re: [PATCH v3 2/5] Docs: usb: update comment and code near decrement our usage count for the device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Dec 2021 10:30:26 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Philipp Hortmann <philipp.g.hortmann@...il.com>, corbet@....net,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     linux-usb@...r.kernel.org, gregkh@...uxfoundation.org
Subject: Re: [PATCH v3 2/5] Docs: usb: update comment and code near decrement
 our usage count for the device

On 06.12.21 21:57, Philipp Hortmann wrote:

> Update comment: decrement our usage count ..
> and code according to usb-skeleton.c

Hi,

and that is exactly the problem, I am afraid.
Your patch would be correct if the underlying code were correct.

>  
> -    /* decrement our usage count for the device */
> -    --skel->open_count;
> +    /* decrement the count on our device */
> +    kref_put(&dev->kref, skel_delete);
>  
>  
>  One of the more difficult problems that USB drivers must be able to

I am sorry but the code in usb-skel.c is wrong. You grab a reference
in skel_open():

        /* increment our usage count for the device */
        kref_get(&dev->kref);

which is good, but in skel_release() we do:

        /* decrement the count on our device */
        kref_put(&dev->kref, skel_delete);

unconditionally.

Think this through:

- Device is plugged in -> device node and internal data is created
- open() called -> kref_get(), we get a reference
- close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:

static void skel_delete(struct kref *kref)
{
        struct usb_skel *dev = to_skel_dev(kref);

        usb_free_urb(dev->bulk_in_urb);
        usb_put_intf(dev->interface);
        usb_put_dev(dev->udev);
        kfree(dev->bulk_in_buffer);
        kfree(dev);
}

with intfdata left intact.

- open() is called again -> We are following a dangling pointer into cloud cuckoo land.

Unfortunately this code is older than git, so I cannot just send a revert.
What to do?

	Regards
		Oliver