Re: [PATCH] udf: limit the maximum number of allocation extents

[Jan Kara <jack@suse.cz>]

On Fri 11-12-15 15:54:16, Vegard Nossum wrote:
> Hit this kernel hang too while fuzzing. Please see this as a tentative
> patch indicating where the problem is -- I don't really know much about
> UDF or what an allocation extent is or whether there are more problems
> in the same neighbourhood. It looks like udf_truncate_extents() might
> also have a similar problem?
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> Cc: stable@vger.kernel.org
> Cc: Jan Kara <jack@suse.com>
> Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  fs/udf/inode.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git fs/udf/inode.c fs/udf/inode.c
> index 8d0b3ad..e1875f5 100644
> --- fs/udf/inode.c
> +++ fs/udf/inode.c
> @@ -2047,13 +2047,26 @@ void udf_write_aext(struct inode *inode, struct extent_position *epos,
>  		epos->offset += adsize;
>  }
>  
> +/*
> + * Maximum number of allocation extents. The chosen number is
> + * arbitrary - just that we hopefully don't limit any real use
> + * but avoid looping for too long on corrupted media.
> + */
> +#define UDF_MAX_AEXT_NESTING 4096
> +

I don't like to limit the number of indirect extents in a file. Although
4096 is quite a bit, there is a real chance it won't be enough for some
usecases (although I agree that such usecases would be very slow with the
current implementation of UDF anyway). What I'd prefer is to limit the
number of indirect extents to maximum possible sane number. Something like:

(inode->i_size >> inode->i_blkbits) / (extents_per_block) + 1

That way we are sure we don't limit any real use case and we also avoid
infinite loops.

								Honza

>  int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  		     struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
>  {
>  	int8_t etype;
> +	unsigned int indirections = 0;
>  
>  	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
>  	       (EXT_NEXT_EXTENT_ALLOCDECS >> 30)) {
> +		if (++indirections > UDF_MAX_AEXT_NESTING) {
> +			udf_err(inode->i_sb, "too many AEXTs (max %u supported)\n", UDF_MAX_AEXT_NESTING);
> +			return -1;
> +		}
> +
>  		int block;
>  		epos->block = *eloc;
>  		epos->offset = sizeof(struct allocExtDesc);
> -- 
> 1.9.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR