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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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.