Re: [PATCH] udf: fix OOB read for a crafted unallocSpaceEntry

[Jan Kara]

On Sun 26-07-26 15:25:09, Baul Lee wrote:
> udf_read_inode() sanity-checks the on-disk allocation-descriptor length
> before it is used:
> 
> 	if (iinfo->i_lenEAttr > bs || iinfo->i_lenAlloc > bs)
> 		goto out;
> 	/* Now do exact checks */
> 	if (udf_file_entry_alloc_offset(inode) + iinfo->i_lenAlloc > bs)
> 		goto out;
> 
> but the TAG_IDENT_USE branch sets i_lenAlloc from the on-disk
> lengthAllocDescs and returns before ever reaching them.  A USE inode
> therefore keeps a completely unvalidated le32 there, while i_data is
> allocated at only bs - sizeof(struct unallocSpaceEntry) bytes.
> 
> udf_current_aext() computes, in its in-ICB branch,
> 
> 	alen = udf_file_entry_alloc_offset(inode) + iinfo->i_lenAlloc;
> 
> with no clamp and passes alen to udf_get_fileshortad() as maxoffset.
> That helper only checks (*offset + sizeof(struct short_ad)) > maxoffset,
> so an inflated i_lenAlloc lets the walk step past the end of i_data and
> dereference sa->extLength in adjacent slab memory.
> 
> A Type-1 partition whose unallocSpaceTable extent is non-zero loads the
> USE table inode at mount and sets UDF_PART_FLAG_UNALLOC_TABLE.  An
> unprivileged statfs(2) then reaches udf_count_free_table() ->
> udf_next_aext() -> udf_current_aext(), and KASAN reports a
> slab-out-of-bounds read just past the 472-byte i_data allocation.
> 
> Apply the same exact check on the USE path, before i_data is allocated
> and walked.  ret is still -EIO there, so the offending inode fails to
> load.
> 
> Discovered by XBOW, triaged by Baul Lee <baul.lee@xxxxxxxx>
> 
> Fixes: 23b133bdc452 ("udf: Check length of extended attributes and allocation descriptors")
> Reported-by: Federico Kirschbaum <federico.kirschbaum@xxxxxxxx>
> Reported-by: Baul Lee <baul.lee@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Baul Lee <baul.lee@xxxxxxxx>

Thanks for the patch but commit b8228f59dc58 ("udf: bound lengthAllocDescs
from unallocated space entry") in my tree should already handle this.

								Honza

> ---
>  fs/udf/inode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index c97914aa8d8b..8a0c9ea31a11 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1475,6 +1475,8 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
>  		iinfo->i_lenAlloc = le32_to_cpu(
>  				((struct unallocSpaceEntry *)bh->b_data)->
>  				 lengthAllocDescs);
> +		if (udf_file_entry_alloc_offset(inode) + iinfo->i_lenAlloc > bs)
> +			goto out;
>  		ret = udf_alloc_i_data(inode, bs -
>  					sizeof(struct unallocSpaceEntry));
>  		if (ret)
> -- 
> 2.50.1 (Apple Git-155)
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR