On Fri, Sep 23, 2022 at 09:39:39AM -0300, Jason Gunthorpe wrote: > On Fri, Sep 23, 2022 at 12:10:12PM +1000, Dave Chinner wrote: > > > > Jason mentioned a scenario here: > > > > > > https://lore.kernel.org/all/YyuoE8BgImRXVkkO@xxxxxxxxxx/ > > > > > > Multi-thread process where thread1 does open(O_DIRECT)+mmap()+read() and > > > thread2 does memunmap()+close() while the read() is inflight. > > > > And, ah, what production application does this and expects to be > > able to process the result of the read() operation without getting a > > SEGV? > > The read() will do GUP and get a pined page, next the memunmap()/close > will release the inode the VMA was holding open. The read() FD is NOT > a DAX FD. > > We are now UAFing the DAX storage. There is no SEGV. Yes, but what happens *after* the read(). The userspace application now tries to access the mmap() region to access the data that was read, only to find that it's been unmapped. That triggers a SEGV, yes? IOWs, there's nothing *useful* a user application can do with a pattern like this. All it provides is a vector for UAF of the DAX storage. Now replace the read() with write(), and tell me why this can't cause data corruption and/or fatal filesystem corruption that can take the entire system down..... > It is not about sane applications, it is about kernel security against > hostile userspace. Turning this into a UAF doesn't provide any security at all. It makes this measurable worse from a security POV as it provides a channel for data leakage (read() case) or system unstability or compromise (the write() case). > > i.e. The underlying problem here is that memunmap() frees the VMA > > while there are still active task-based references to the pages in > > that VMA. IOWs, the VMA should not be torn down until the O_DIRECT > > read has released all the references to the pages mapped into the > > task address space. > > This is Jan's suggestion, I think we are still far from being able to > do that for O_DIRECT paths. > > Even if you fix the close() this way, doesn't truncate still have the > same problem? It sure does. Also fallocate(). The deja vu is strong right now. If something truncate()s a file, the only safe thing for an application that is using fsdax to directly access the underying storage is to unmap the file and remap it once the layout change operation has completed. We've been doing this safely with pNFS for remote RDMA-based direct access to the storage hardware for years. We have the layout lease infrastructure already there for it... I've pointed this out every time this conversation comes up. We have a solution for this problem pretty much ready to go - it just needs a UAPI to be defined for it. i.e. nothing has changed in the past 5 years - we have the same problems, we have the same solutions ready to be hooked up and used.... > At the end of the day the rule is a DAX page must not be re-used until > its refcount is 0. At some point the FS should wait for. The page is *not owned by DAX*. How many times do I have to say that FSDAX != DAX. The *storage media* must not be reused until the filesystem says it can be reused. And for that to work, nothing is allowed to keep an anonymous, non-filesystem reference to the storage media. It has nothing to do with struct page reference counts, and everything to do with ensuring that filesystem objects are correctly referenced while the storage media is in direct use by an application. I gave up on FSDAX years ago because nobody was listening to me. Here we are again, years down the track, with exactly the same issues as we had years ago, with exactly the same people repeating the same arguments for and against fixing the page reference problems. I don't have time to repeat history all over again, so I'm going to walk away from this train-wreck again so I can maintain some semblence of my remaining sanity.... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
