| Message ID | 20250318003028.3330599-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] fuse: fix uring race condition for null dereference of fc | expand |
On 3/18/25 01:30, Joanne Koong wrote: > There is a race condition leading to a kernel crash from a null > dereference when attemping to access fc->lock in > fuse_uring_create_queue(). fc may be NULL in the case where another > thread is creating the uring in fuse_uring_create() and has set > fc->ring but has not yet set ring->fc when fuse_uring_create_queue() > reads ring->fc. There is another race condition as well where in > fuse_uring_register(), ring->nr_queues may still be 0 and not yet set > to the new value when we compare qid against it. > > This fix sets fc->ring only after ring->fc and ring->nr_queues have been > set, which guarantees now that ring->fc is a proper pointer when any > queues are created and ring->nr_queues reflects the right number of > queues if ring is not NULL. We must use smp_store_release() and > smp_load_acquire() semantics to ensure the ordering will remain correct > where fc->ring is assigned only after ring->fc and ring->nr_queues have > been assigned. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands") > > --- > > Changes between v2 -> v3: > * v2 implementation still has race condition for ring->nr_queues > *link to v2: https://lore.kernel.org/linux-fsdevel/20250314205033.762641-1-joannelkoong@gmail.com/ > > Changes between v1 -> v2: > * v1 implementation may be reordered by compiler (Bernd) > * link to v1: https://lore.kernel.org/linux-fsdevel/20250314191334.215741-1-joannelkoong@gmail.com/ > > --- > fs/fuse/dev_uring.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index ab8c26042aa8..97e6d31479e0 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -235,11 +235,11 @@ static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc) > > init_waitqueue_head(&ring->stop_waitq); > > - fc->ring = ring; > ring->nr_queues = nr_queues; > ring->fc = fc; > ring->max_payload_sz = max_payload_size; > atomic_set(&ring->queue_refs, 0); > + smp_store_release(&fc->ring, ring); > > spin_unlock(&fc->lock); > return ring; > @@ -1068,7 +1068,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd, > unsigned int issue_flags, struct fuse_conn *fc) > { > const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe); > - struct fuse_ring *ring = fc->ring; > + struct fuse_ring *ring = smp_load_acquire(&fc->ring); > struct fuse_ring_queue *queue; > struct fuse_ring_ent *ent; > int err; I was actually debating with myself that smp_load_acquire() on Friday. I think we do not need it, because if the ring is not found, it will go into the spin lock. But it does not hurt either and it might cleaner to have a pair of smp_store_release() and smp_load_acquire(). Reviewed-by: Bernd Schubert <bschubert@ddn.com>
On Tue, 18 Mar 2025 at 01:30, Joanne Koong <joannelkoong@gmail.com> wrote: > > There is a race condition leading to a kernel crash from a null > dereference when attemping to access fc->lock in > fuse_uring_create_queue(). fc may be NULL in the case where another > thread is creating the uring in fuse_uring_create() and has set > fc->ring but has not yet set ring->fc when fuse_uring_create_queue() > reads ring->fc. There is another race condition as well where in > fuse_uring_register(), ring->nr_queues may still be 0 and not yet set > to the new value when we compare qid against it. > > This fix sets fc->ring only after ring->fc and ring->nr_queues have been > set, which guarantees now that ring->fc is a proper pointer when any > queues are created and ring->nr_queues reflects the right number of > queues if ring is not NULL. We must use smp_store_release() and > smp_load_acquire() semantics to ensure the ordering will remain correct > where fc->ring is assigned only after ring->fc and ring->nr_queues have > been assigned. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands") Acked-by: Miklos Szeredi <mszeredi@redhat.com> Christian, can you please queue this fix up for 6.14-final? Thanks, Miklos
On Tue, Mar 18, 2025 at 3:38 AM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 3/18/25 01:30, Joanne Koong wrote: > > There is a race condition leading to a kernel crash from a null > > dereference when attemping to access fc->lock in > > fuse_uring_create_queue(). fc may be NULL in the case where another > > thread is creating the uring in fuse_uring_create() and has set > > fc->ring but has not yet set ring->fc when fuse_uring_create_queue() > > reads ring->fc. There is another race condition as well where in > > fuse_uring_register(), ring->nr_queues may still be 0 and not yet set > > to the new value when we compare qid against it. > > > > This fix sets fc->ring only after ring->fc and ring->nr_queues have been > > set, which guarantees now that ring->fc is a proper pointer when any > > queues are created and ring->nr_queues reflects the right number of > > queues if ring is not NULL. We must use smp_store_release() and > > smp_load_acquire() semantics to ensure the ordering will remain correct > > where fc->ring is assigned only after ring->fc and ring->nr_queues have > > been assigned. > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands") > > > > --- > > > > Changes between v2 -> v3: > > * v2 implementation still has race condition for ring->nr_queues > > *link to v2: https://lore.kernel.org/linux-fsdevel/20250314205033.762641-1-joannelkoong@gmail.com/ > > > > Changes between v1 -> v2: > > * v1 implementation may be reordered by compiler (Bernd) > > * link to v1: https://lore.kernel.org/linux-fsdevel/20250314191334.215741-1-joannelkoong@gmail.com/ > > > > --- > > fs/fuse/dev_uring.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > > index ab8c26042aa8..97e6d31479e0 100644 > > --- a/fs/fuse/dev_uring.c > > +++ b/fs/fuse/dev_uring.c > > @@ -235,11 +235,11 @@ static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc) > > > > init_waitqueue_head(&ring->stop_waitq); > > > > - fc->ring = ring; > > ring->nr_queues = nr_queues; > > ring->fc = fc; > > ring->max_payload_sz = max_payload_size; > > atomic_set(&ring->queue_refs, 0); > > + smp_store_release(&fc->ring, ring); > > > > spin_unlock(&fc->lock); > > return ring; > > @@ -1068,7 +1068,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd, > > unsigned int issue_flags, struct fuse_conn *fc) > > { > > const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe); > > - struct fuse_ring *ring = fc->ring; > > + struct fuse_ring *ring = smp_load_acquire(&fc->ring); > > struct fuse_ring_queue *queue; > > struct fuse_ring_ent *ent; > > int err; > > I was actually debating with myself that smp_load_acquire() on Friday. > I think we do not need it, because if the ring is not found, it will > go into the spin lock. But it does not hurt either and it might > cleaner to have a pair of smp_store_release() and smp_load_acquire(). > I think the problem is that if we don't have it, then ring may be non-null but the ring->nr_queues value we read in "if (qid >= ring->nr_queues)" might still be the stale value, which would make the uring registration fail with -EINVAL. Thanks, Joanne > > Reviewed-by: Bernd Schubert <bschubert@ddn.com>
On Mon, 17 Mar 2025 17:30:28 -0700, Joanne Koong wrote: > There is a race condition leading to a kernel crash from a null > dereference when attemping to access fc->lock in > fuse_uring_create_queue(). fc may be NULL in the case where another > thread is creating the uring in fuse_uring_create() and has set > fc->ring but has not yet set ring->fc when fuse_uring_create_queue() > reads ring->fc. There is another race condition as well where in > fuse_uring_register(), ring->nr_queues may still be 0 and not yet set > to the new value when we compare qid against it. > > [...] Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.fixes [1/1] fuse: fix uring race condition for null dereference of fc https://git.kernel.org/vfs/vfs/c/d9ecc77193ca
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index ab8c26042aa8..97e6d31479e0 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -235,11 +235,11 @@ static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc) init_waitqueue_head(&ring->stop_waitq); - fc->ring = ring; ring->nr_queues = nr_queues; ring->fc = fc; ring->max_payload_sz = max_payload_size; atomic_set(&ring->queue_refs, 0); + smp_store_release(&fc->ring, ring); spin_unlock(&fc->lock); return ring; @@ -1068,7 +1068,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd, unsigned int issue_flags, struct fuse_conn *fc) { const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe); - struct fuse_ring *ring = fc->ring; + struct fuse_ring *ring = smp_load_acquire(&fc->ring); struct fuse_ring_queue *queue; struct fuse_ring_ent *ent; int err;
There is a race condition leading to a kernel crash from a null dereference when attemping to access fc->lock in fuse_uring_create_queue(). fc may be NULL in the case where another thread is creating the uring in fuse_uring_create() and has set fc->ring but has not yet set ring->fc when fuse_uring_create_queue() reads ring->fc. There is another race condition as well where in fuse_uring_register(), ring->nr_queues may still be 0 and not yet set to the new value when we compare qid against it. This fix sets fc->ring only after ring->fc and ring->nr_queues have been set, which guarantees now that ring->fc is a proper pointer when any queues are created and ring->nr_queues reflects the right number of queues if ring is not NULL. We must use smp_store_release() and smp_load_acquire() semantics to ensure the ordering will remain correct where fc->ring is assigned only after ring->fc and ring->nr_queues have been assigned. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands") --- Changes between v2 -> v3: * v2 implementation still has race condition for ring->nr_queues *link to v2: https://lore.kernel.org/linux-fsdevel/20250314205033.762641-1-joannelkoong@gmail.com/ Changes between v1 -> v2: * v1 implementation may be reordered by compiler (Bernd) * link to v1: https://lore.kernel.org/linux-fsdevel/20250314191334.215741-1-joannelkoong@gmail.com/ --- fs/fuse/dev_uring.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)