linux-kernel - Re: bcachefs: suspicious mm pointer in struct dio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <69510752-d6f9-4cf1-b93d-dcd249d911ef@kernel.dk>
Date: Wed, 27 Nov 2024 11:09:14 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Jann Horn <jannh@...gle.com>, Kent Overstreet
 <kent.overstreet@...ux.dev>, linux-bcachefs@...r.kernel.org
Cc: kernel list <linux-kernel@...r.kernel.org>,
 Pavel Begunkov <asml.silence@...il.com>, io-uring <io-uring@...r.kernel.org>
Subject: Re: bcachefs: suspicious mm pointer in struct dio_write

On 11/27/24 9:57 AM, Jann Horn wrote:
> Hi!
> 
> In fs/bcachefs/fs-io-direct.c, "struct dio_write" contains a pointer
> to an mm_struct. This pointer is grabbed in bch2_direct_write()
> (without any kind of refcount increment), and used in
> bch2_dio_write_continue() for kthread_use_mm()/kthread_unuse_mm()
> which are used to enable userspace memory access from kthread context.
> I believe kthread_use_mm()/kthread_unuse_mm() require that the caller
> guarantees that the MM hasn't gone through exit_mmap() yet (normally
> by holding an mmget() reference).
> 
> If we reach this codepath via io_uring, do we have a guarantee that
> the mm_struct that called bch2_direct_write() is still alive and
> hasn't yet gone through exit_mmap() when it is accessed from
> bch2_dio_write_continue()?
> 
> I don't know the async direct I/O codepath particularly well, so I
> cc'ed the uring maintainers, who probably know this better than me.

I _think_ this is fine as-is, even if it does look dubious and bcachefs
arguably should grab an mm ref for this just for safety to avoid future
problems. The reason is that bcachefs doesn't set FMODE_NOWAIT, which
means that on the io_uring side it cannot do non-blocking issue of
requests. This is slower as it always punts to an io-wq thread, which
shares the same mm. Hence if the request is alive, there's always a
thread with the same mm alive as well.

Now if FMODE_NOWAIT was set, then the original task could exit. I'd need
to dig a bit deeper to verify that would always be safe and there's not
a of time today with a few days off in the US looming, so I'll defer
that to next week. It certainly would be fine with an mm ref grabbed.

-- 
Jens Axboe