linux-kernel - Re: [PATCH] firewire: nosy: Fix a use-after-free bug in nosy

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] [day] [month] [year] [list]

Message-ID: <CAHk-=wh+sw_cYnL2XyuhknOpOh1jEPURg-W=jS2CyW2=ud+zog@mail.gmail.com>
Date:   Sat, 3 Apr 2021 09:05:40 -0700
From:   Linus Torvalds <torvalds@...uxfoundation.org>
To:     Zheyu Ma <zheyuma97@...il.com>
Cc:     stefanr@...6.in-berlin.de, linux1394-devel@...ts.sourceforge.net,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Security Officers <security@...nel.org>,
        Greg Kroah-Hartman <greg@...ah.com>
Subject: Re: [PATCH] firewire: nosy: Fix a use-after-free bug in nosy_ioctl()

On Fri, Apr 2, 2021 at 11:59 PM Zheyu Ma <zheyuma97@...il.com> wrote:
>
>         case NOSY_IOC_START:
> +               list_for_each_entry(tmp, &client->lynx->client_list, link)
> +                       if (tmp == client)
> +                               return -EINVAL;

I don't think this is safe.

You are doing this list traversal outside the lock that protects it,
which it taken a line later:

>                 spin_lock_irq(client_list_lock);
>                 list_add_tail(&client->link, &client->lynx->client_list);
>                 spin_unlock_irq(client_list_lock);

so the locking is wrong.

However, I think that the proper fix is not just to move the code
inside the locked region (which makes the error handling a bit more
complex than just a return, of course), but to actually instead of
traversing the list, just look if the "client->link" list is empty.

That's what some other parts of that driver already do (ie
nosy_poll()), so I think that ->link field is already always
initialized properly (and it looks like all the list removal is using
"list_del_init()" to initialize it after removing it from a list.

So I think the patch should be something along the lines of

    --- a/drivers/firewire/nosy.c
    +++ b/drivers/firewire/nosy.c
    @@ -346,6 +346,7 @@ nosy_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
        struct client *client = file->private_data;
        spinlock_t *client_list_lock = &client->lynx->client_list_lock;
        struct nosy_stats stats;
    +   int ret;

        switch (cmd) {
        case NOSY_IOC_GET_STATS:
    @@ -360,11 +361,15 @@ nosy_ioctl(struct file *file,
                        return 0;

        case NOSY_IOC_START:
    +           ret = -EBUSY;
                spin_lock_irq(client_list_lock);
    -           list_add_tail(&client->link, &client->lynx->client_list);
    +           if (list_empty(&client->link)) {
    +                   list_add_tail(&client->link,
&client->lynx->client_list);
    +                   ret = 0;
    +           }
                spin_unlock_irq(client_list_lock);

    -           return 0;
    +           return ret;

        case NOSY_IOC_STOP:
                spin_lock_irq(client_list_lock);

instead. The above is obviously white-space damaged (on purpose - I
don't want to take credit for this patch, I didn't find the problem,
and I have not tested the above in any shape or form).

Zheyu Ma, does something like that work for you?

Comments? Anybody else?

        Linus