Re: [PATCH] firewire: nosy: Fix a use-after-free bug in nosy_ioctl()

[Linus Torvalds]

On Fri, Apr 2, 2021 at 11:59 PM Zheyu Ma <zheyuma97@xxxxxxxxx> 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