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

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

[Vegard Nossum]

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
+
 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

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

[Jan Kara]

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

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

[Jan Kara]

On Mon 14-12-15 21:34:26, Jan Kara wrote:
> 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.

In the end I have realized that this is actually only about the case where
indirect extent has the first extent which is also indirect. Such case
shouldn't happen in practice at all so I have even reduced the limit and
committed the patch.

								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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR