lkml.org 
[lkml]   [2021]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] firewire: nosy: Fix a use-after-free bug in nosy_ioctl()
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

\
 
 \ /
  Last update: 2021-04-03 18:06    [W:0.042 / U:4.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site