Re: [PATCH] udf: bound lengthAllocDescs from unallocated space entry

[Jan Kara]

On Fri 17-07-26 11:40:19, Jay Vadayath wrote:
> udf_read_inode() copies the on-disk lengthAllocDescs field of a USE
> (unallocSpaceEntry) inode into iinfo->i_lenAlloc without checking that
> it fits in the i_data buffer that is subsequently allocated for the
> inode. udf_count_free_table(), called from udf_statfs(), then walks the
> allocation descriptor array up to i_lenAlloc bytes, so a crafted UDF
> image with lengthAllocDescs larger than (blocksize - sizeof(struct
> unallocSpaceEntry)) causes udf_get_fileshortad() to read past the end
> of the kmalloc'd i_data buffer.
> 
> KASAN report from mounting a crafted UDF image and calling statfs()
> from an unprivileged process:
> 
>   BUG: KASAN: slab-out-of-bounds in udf_get_fileshortad+0x126/0x130
>   Read of size 4 at addr ffff8880042137d8 by task poc/65
>   Call Trace:
>    dump_stack_lvl+0x53/0x70
>    print_report+0xce/0x610
>    kasan_report+0xce/0x100
>    udf_get_fileshortad+0x126/0x130
>    udf_current_aext+0x3c4/0xa10
>    udf_next_aext+0x241/0x440
>    udf_statfs+0xb7d/0x11c0
>    statfs_by_dentry+0x117/0x1e0
>    user_statfs+0xac/0x130
>    __do_sys_statfs+0x80/0xe0
>    do_syscall_64+0x102/0x5a0
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Reject USE inodes whose lengthAllocDescs would place descriptors past
> the end of the i_data buffer, mirroring the checks the rest of the UDF
> code performs on descriptor lengths.
> 
> This bug was discovered by Artiphishell's vTriage pipeline, which
> generated a userspace reproducer that reliably triggers the KASAN
> report on an unpatched kernel. The fix below was drafted with the
> Claude coding assistant; a userspace reproducer (and the crafted UDF
> image) is available on request.
> 
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Jay Vadayath <jay@xxxxxxxxxxxxxxxx>

Thanks. I've added the patch (with minor edits) to my tree.

								Honza

> 
> ---
>  fs/udf/inode.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1475,6 +1475,13 @@
>  		iinfo->i_lenAlloc = le32_to_cpu(
>  				((struct unallocSpaceEntry *)bh->b_data)->
>  				 lengthAllocDescs);
> +		/*
> +		 * Sanity check the length of allocation descriptors so we do
> +		 * not read past the end of the allocated i_data buffer when
> +		 * walking them later (e.g. from udf_count_free_table()).
> +		 */
> +		if (iinfo->i_lenAlloc > bs - sizeof(struct unallocSpaceEntry))
> +			goto out;
>  		ret = udf_alloc_i_data(inode, bs -
>  					sizeof(struct unallocSpaceEntry));
>  		if (ret)
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR