Re: [PATCH] fs: aio: reject partial mremap to avoid Null-pointer-dereference error

On Sat 18-04-26 14:06:34, Zizhi Wo wrote:
> From: Zizhi Wo <[email protected]>
> 
> [BUG]
> Recently, our internal syzkaller testing uncovered a null pointer
> dereference issue:
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> ...
> [   51.111664]  filemap_read_folio+0x25/0xe0
> [   51.112410]  filemap_fault+0xad7/0x1250
> [   51.113112]  __do_fault+0x4b/0x460
> [   51.113699]  do_pte_missing+0x5bc/0x1db0
> [   51.114250]  ? __pte_offset_map+0x23/0x170
> [   51.114822]  __handle_mm_fault+0x9f8/0x1680
> ...
> Crash analysis showed the file involved was an AIO ring file. The
> phenomenon triggered is the same as the issue described in [1].
> 
> [CAUSE]
> Consider the following scenario: userspace sets up an AIO context via
> io_setup(), which creates a VMA covering the entire ring buffer. Then
> userspace calls mremap() with the AIO ring address as the source, a smaller
> old_len (less than the full ring size), MREMAP_MAYMOVE set, and without
> MREMAP_DONTUNMAP. The kernel will relocate the requested portion to a new
> destination address.
> 
> During this move, __split_vma() splits the original AIO ring VMA. The
> requested portion is unmapped from the source and re-established at the
> destination, while the remainder stays at the original source address as
> an orphan VMA. The aio_ring_mremap() callback fires on the new destination
> VMA, updating ctx->mmap_base to the destination address. But the callback
> is unaware that only a partial region was moved and that an orphan VMA
> still exists at the source:
> 
>   source(AIO):
>   +-------------------+---------------------+
>   |  moved to dest    |  orphan VMA (AIO)   |
>   +-------------------+---------------------+
>   A                 A+partial_len        A+ctx->mmap_size
> 
>   dest:
>   +-------------------+
>   |  moved VMA (AIO)  |
>   +-------------------+
>   B                 B+partial_len
> 
> Later, io_destroy() calls vm_munmap(ctx->mmap_base, ctx->mmap_size), which
> unmaps the destination. This not only fails to unmap the orphan VMA at the
> source, but also overshoots the destination VMA and may unmap unrelated
> mappings adjacent to it! After put_aio_ring_file() calls truncate_setsize()
> to remove all pages from the pagecache, any subsequent access to the orphan
> VMA triggers filemap_fault(), which calls a_ops->read_folio(). Since aio
> does not implement read_folio, this results in a NULL pointer dereference.
> 
> [FIX]
> Note that expanding mremap (new_len > old_len) is already rejected because
> AIO ring VMAs are created with VM_DONTEXPAND. The only problematic case is
> a partial move where "old_len == new_len" but both are smaller than the
> full ring size.
> 
> Fix this by checking in aio_ring_mremap() that the new VMA covers the
> entire ring. This ensures the AIO ring is always moved as a whole,
> preventing orphan VMAs and the subsequent crash.
> 
> [1]: https://lore.kernel.org/all/[email protected]/
> 
> Signed-off-by: Zizhi Wo <[email protected]>

Looks good! Thanks! Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/aio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index a07bdd1aaaa6..48d049ff5267 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -369,7 +369,8 @@ static int aio_ring_mremap(struct vm_area_struct *vma)
>  
>  		ctx = rcu_dereference(table->table[i]);
>  		if (ctx && ctx->aio_ring_file == file) {
> -			if (!atomic_read(&ctx->dead)) {
> +			if (!atomic_read(&ctx->dead) &&
> +			    (ctx->mmap_size == (vma->vm_end - vma->vm_start))) {
>  				ctx->user_id = ctx->mmap_base = vma->vm_start;
>  				res = 0;
>  			}
> -- 
> 2.39.2
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to [email protected].  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"[email protected]">[email protected]</a>