Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1617433116-5930-1-git-send-email-zheyuma97@gmail.com>
In-Reply-To: <1617433116-5930-1-git-send-email-zheyuma97@gmail.com>
From:   Linus Torvalds <torvalds@linuxfoundation.org>
Date:   Sat, 3 Apr 2021 09:05:40 -0700
Message-ID: <CAHk-=wh+sw_cYnL2XyuhknOpOh1jEPURg-W=jS2CyW2=ud+zog@mail.gmail.com>
Subject: Re: [PATCH] firewire: nosy: Fix a use-after-free bug in nosy_ioctl()
To:     Zheyu Ma <zheyuma97@gmail.com>
Cc:     stefanr@s5r6.in-berlin.de, linux1394-devel@lists.sourceforge.net,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Security Officers <security@kernel.org>,
        Greg Kroah-Hartman <greg@kroah.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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