From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752245AbcAEPtv (ORCPT ); Tue, 5 Jan 2016 10:49:51 -0500 Received: from mx2.suse.de ([195.135.220.15]:43052 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751755AbcAEPts (ORCPT ); Tue, 5 Jan 2016 10:49:48 -0500 Date: Tue, 5 Jan 2016 16:49:36 +0100 From: Jan Kara To: Vegard Nossum Cc: Jan Kara , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Quentin Casasnovas , Andrew Morton Subject: Re: [PATCH] udf: limit the maximum number of allocation extents Message-ID: <20160105154936.GA18604@quack.suse.cz> References: <1449845656-8974-1-git-send-email-vegard.nossum@oracle.com> <20151214203426.GO8474@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151214203426.GO8474@quack.suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > Cc: stable@vger.kernel.org > > Cc: Jan Kara > > Cc: Quentin Casasnovas > > Cc: Andrew Morton > > --- > > 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 > SUSE Labs, CR -- Jan Kara SUSE Labs, CR