From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932162AbbLNJwe (ORCPT ); Mon, 14 Dec 2015 04:52:34 -0500 Received: from mx2.suse.de ([195.135.220.15]:34895 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752691AbbLNJwc (ORCPT ); Mon, 14 Dec 2015 04:52:32 -0500 Date: Mon, 14 Dec 2015 10:52:29 +0100 From: Jan Kara To: Vegard Nossum Cc: Jan Kara , linux-kernel@vger.kernel.org, quentin.casasnovas@oracle.com Subject: Re: [PATCH] udf: limit the maximum number of TD redirections Message-ID: <20151214095229.GB8474@quack.suse.cz> References: <1449764021-20609-1-git-send-email-vegard.nossum@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449764021-20609-1-git-send-email-vegard.nossum@oracle.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 10-12-15 17:13:41, Vegard Nossum wrote: > Filesystem fuzzing revealed that we could get stuck in the > udf_process_sequence() loop. > > The maximum limit was chosen arbitrarily but fixes the problem I saw. Process nit: The patch is missing your Signed-off-by. > diff --git fs/udf/super.c fs/udf/super.c > index 81155b9..fd45537 100644 > --- fs/udf/super.c > +++ fs/udf/super.c > @@ -1586,6 +1586,13 @@ static void udf_load_logicalvolint(struct super_block *sb, struct kernel_extent_ > } > > /* > + * Maximum number of Terminating Descriptor redirections. The chosen number is > + * arbitrary - just that we hopefully don't limit any real use of rewritten > + * inode on write-once media but avoid looping for too long on corrupted media. > + */ > +#define UDF_MAX_TD_NESTING 64 > + > +/* > * Process a main/reserve volume descriptor sequence. > * @block First block of first extent of the sequence. > * @lastblock Lastblock of first extent of the sequence. > @@ -1609,6 +1616,7 @@ static noinline int udf_process_sequence( > uint16_t ident; > long next_s = 0, next_e = 0; > int ret; > + unsigned int indirections = 0; > > memset(vds, 0, sizeof(struct udf_vds_record) * VDS_POS_LENGTH); > > @@ -1679,6 +1687,12 @@ static noinline int udf_process_sequence( > } > break; > case TAG_IDENT_TD: /* ISO 13346 3/10.9 */ > + if (++indirections > UDF_MAX_TD_NESTING) { > + udf_err(sb, "too many TDs (max %u supported)\n", UDF_MAX_TD_NESTING); > + brelse(bh); > + return -EIO; > + } > + > vds[VDS_POS_TERMINATING_DESC].block = block; > if (next_e) { But this doesn't really help much. When we speak about corrupted media, then most likely we hit a case where descriptor CRCs won't match and so we return EIO. How exactly did your fuzzing corrupt the filesystem? I suppose it hardly created long sequence of correct VDP descriptors just by pure "luck". When we speak about malicious media, then we are free to generate volume descriptor sequence of valid descriptors that doesn't have a single terminating descriptor and so we will read the whole area for volume descriptor sequence described in the anchor anyway. So I don't see the case where your patch would significantly help. Frankly a malicious volume that makes the kernel read a lot of disk when mounting fails to excite me much. But what we could do is to make mount process killable when reading the descriptor sequence. That should at least allow sysadmin to terminate mount of malicious media. Hmm? Honza Honza -- Jan Kara SUSE Labs, CR