[PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Jia-Ju Bai]

In drivers/fsi/fsi-sbefifo.c, the functions sbefifo_user_release(), 
sbefifo_user_read() and sbefifo_user_write() may be concurrently executed.

sbefifo_user_release()
  sbefifo_release_command()
    vfree(user->pending_cmd);

sbefifo_user_read()
  mutex_lock();
  rc = __sbefifo_submit(sbefifo, user->pending_cmd, ...);

sbefifo_user_write()
  mutex_lock();
  user->pending_cmd = user->cmd_page;
  user->pending_cmd = vmalloc(len);

Thus, possible concurrency use-after-free bugs may occur in
sbefifo_user_release().

To fix these bugs, the calls to mutex_lock() and mutex_unlock() are
added in sbefifo_user_release().


Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/fsi/fsi-sbefifo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index d92f5b87c251..e278a9014b8f 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -900,8 +900,10 @@ static int sbefifo_user_release(struct inode *inode, struct file *file)
 	if (!user)
 		return -EINVAL;
 
+	mutex_lock(&user->file_lock);
 	sbefifo_release_command(user);
 	free_page((unsigned long)user->cmd_page);
+	mutex_unlock(&user->file_lock);
 	kfree(user);
 
 	return 0;
-- 
2.17.0

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[David Howells]

Jia-Ju Bai <baijiaju1990@gmail.com> wrote:

> +	mutex_lock(&user->file_lock);
>  	sbefifo_release_command(user);
>  	free_page((unsigned long)user->cmd_page);
> +	mutex_unlock(&user->file_lock);

It shouldn't be necessary to do the free_page() call inside the locked
section.

David

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Benjamin Herrenschmidt]

On Wed, 2019-01-02 at 09:34 +0000, David Howells wrote:
> Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
> 
> > +	mutex_lock(&user->file_lock);
> >  	sbefifo_release_command(user);
> >  	free_page((unsigned long)user->cmd_page);
> > +	mutex_unlock(&user->file_lock);
> 
> It shouldn't be necessary to do the free_page() call inside the locked
> section.

True. However, I didn't realize read/write could be concurrent with
release so we have another problem.

I assume when release is called, no new read/write can be issued, I am
correct ? So all we have to protect against is a read/write that has
started prior to release being called, right ?

In that case, what can happen is that release() wins the race on the
mutex, frees everything, then read or write starts using feed stuff.

This is nasty to fix because the mutex is in the user structure,
so even looking at the mutex is racy if release is called.

The right fix, would be, I think, for "user" (pointed to by file-
>private_data) to be protected by a kref. That doesn't close it
completely as the free in release() can still lead to the structure
becoming re-used before read/write tries to get the kref but after it
has NULL checked the private data.

So to make that solid, I would also RCU-defer the actual freeing and
use RCU around dereferencing file->private_data

Now, I yet have to see other chardevs do any of the above, do that mean
they are all hopelessly racy ?

Cheers,
Ben.

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Benjamin Herrenschmidt]

On Thu, 2019-01-03 at 14:27 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2019-01-02 at 09:34 +0000, David Howells wrote:
> > Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
> > 
> > > +	mutex_lock(&user->file_lock);
> > >  	sbefifo_release_command(user);
> > >  	free_page((unsigned long)user->cmd_page);
> > > +	mutex_unlock(&user->file_lock);
> > 
> > It shouldn't be necessary to do the free_page() call inside the locked
> > section.
> 
> True. However, I didn't realize read/write could be concurrent with
> release so we have another problem.
> 
> I assume when release is called, no new read/write can be issued, I am
> correct ? So all we have to protect against is a read/write that has
> started prior to release being called, right ?

Hrm... looking briefly at the vfs, read/write are wrapped in
fdget/fdput, so release shouldn't happen concurrently or am I missing
something here ?

Cheers,
Ben.

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Benjamin Herrenschmidt]

On Wed, 2018-12-26 at 21:56 +0800, Jia-Ju Bai wrote:
> In drivers/fsi/fsi-sbefifo.c, the functions sbefifo_user_release(), 
> sbefifo_user_read() and sbefifo_user_write() may be concurrently executed.

So after refreshing my mind, looking at the code and talking with Al, I
really dont' see what race you are trying to fix here.

read/write should never be concurrent with release for a given file and
the stuff we are protecting here is local to the file instance.

Do you have an actual problem you observed ?

Cheers,
Ben.

> sbefifo_user_release()
>   sbefifo_release_command()
>     vfree(user->pending_cmd);
> 
> sbefifo_user_read()
>   mutex_lock();
>   rc = __sbefifo_submit(sbefifo, user->pending_cmd, ...);
> 
> sbefifo_user_write()
>   mutex_lock();
>   user->pending_cmd = user->cmd_page;
>   user->pending_cmd = vmalloc(len);
> 
> Thus, possible concurrency use-after-free bugs may occur in
> sbefifo_user_release().
> 
> To fix these bugs, the calls to mutex_lock() and mutex_unlock() are
> added in sbefifo_user_release().
> 
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index d92f5b87c251..e278a9014b8f 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -900,8 +900,10 @@ static int sbefifo_user_release(struct inode *inode, struct file *file)
>  	if (!user)
>  		return -EINVAL;
>  
> +	mutex_lock(&user->file_lock);
>  	sbefifo_release_command(user);
>  	free_page((unsigned long)user->cmd_page);
> +	mutex_unlock(&user->file_lock);
>  	kfree(user);
>  
>  	return 0;

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Jia-Ju Bai]

On 2019/1/4 8:47, Benjamin Herrenschmidt wrote:
> On Wed, 2018-12-26 at 21:56 +0800, Jia-Ju Bai wrote:
>> In drivers/fsi/fsi-sbefifo.c, the functions sbefifo_user_release(),
>> sbefifo_user_read() and sbefifo_user_write() may be concurrently executed.
> So after refreshing my mind, looking at the code and talking with Al, I
> really dont' see what race you are trying to fix here.
>
> read/write should never be concurrent with release for a given file and
> the stuff we are protecting here is local to the file instance.
>
> Do you have an actual problem you observed ?
>

Thanks for the reply.

In fact, this report is found by a static tool written by myself, 
instead of real execution.
My tool found that in some drivers, for the structure "struct 
file_operations", the code in intetrfaces ".read" , "write" and 
".release" are protected by the same lock.
The functions kcs_bmc_read(), kcs_bmc_write() and kcs_bmc_release() are 
examples.
Thus, my tool inferred that the intetrfaces ".read" , "write" and 
".release" of "struct file_operations" can be concurrently executed, and 
generated this report.
I manually checked this report, but I was not very sure of it, so I 
marked it as a "possible bug" and reported it.

 From your message, now I know my report is false, and ".read" , "write" 
cannot be concurrently executed with ".release" for a given file.
Sorry for my false report, and thanks for your message.


Best wishes,
Jia-Ju Bai

Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user_release

[Benjamin Herrenschmidt]

On Fri, 2019-01-04 at 10:26 +0800, Jia-Ju Bai wrote:
> 
> On 2019/1/4 8:47, Benjamin Herrenschmidt wrote:
> > On Wed, 2018-12-26 at 21:56 +0800, Jia-Ju Bai wrote:
> > > In drivers/fsi/fsi-sbefifo.c, the functions sbefifo_user_release(),
> > > sbefifo_user_read() and sbefifo_user_write() may be concurrently executed.
> > So after refreshing my mind, looking at the code and talking with Al, I
> > really dont' see what race you are trying to fix here.
> > 
> > read/write should never be concurrent with release for a given file and
> > the stuff we are protecting here is local to the file instance.
> > 
> > Do you have an actual problem you observed ?
> > 
> 
> Thanks for the reply.
> 
> In fact, this report is found by a static tool written by myself, 
> instead of real execution.
> My tool found that in some drivers, for the structure "struct 
> file_operations", the code in intetrfaces ".read" , "write" and 
> ".release" are protected by the same lock.
> The functions kcs_bmc_read(), kcs_bmc_write() and kcs_bmc_release() are 
> examples.
> Thus, my tool inferred that the intetrfaces ".read" , "write" and 
> ".release" of "struct file_operations" can be concurrently executed, and 
> generated this report.
> I manually checked this report, but I was not very sure of it, so I 
> marked it as a "possible bug" and reported it.

So what happens is that they cannot be executed concurrently for a
given struct file. But they can be for separate files.

In the fsi-sbefifo case, all of the data and the lock are part of a
private structure which is allocated in open() and thus is per-file
instance, so there should be no race.

In the example you gave, kcs_bmc.c, the data and lock are part of a
per-device (struct kcs_bmc) and thus shared by all file instances. So
in that case, the race does exist.

>  From your message, now I know my report is false, and ".read" , "write" 
> cannot be concurrently executed with ".release" for a given file.
> Sorry for my false report, and thanks for your message.

Right, your tool is valuable as pre-screening but you need in addition
to check (probably manually) whether the data accessed (and lock) are
shared by multiple open file instances or are entirely local to a given
file instance.

Cheers,
Ben.