From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
Received: from mail-pg0-f49.google.com ([74.125.83.49]:33667 "EHLO
        mail-pg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751965AbcKRUCt (ORCPT
        <rfc822;stable@vger.kernel.org>); Fri, 18 Nov 2016 15:02:49 -0500
Received: by mail-pg0-f49.google.com with SMTP id 3so106237232pgd.0
        for <stable@vger.kernel.org>; Fri, 18 Nov 2016 12:02:49 -0800 (PST)
Date: Fri, 18 Nov 2016 12:02:46 -0800
From: Eric Biggers <ebiggers@google.com>
To: Theodore Ts'o <tytso@mit.edu>
Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>, kernel@kyup.com,
        bp@alien8.de, stable@vger.kernel.org
Subject: Re: [PATCH 1/4] ext4: sanity check the block and cluster size at
 mount time
Message-ID: <20161118200246.GB73496@google.com>
References: <20161118183842.25682-1-tytso@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161118183842.25682-1-tytso@mit.edu>
Sender: stable-owner@vger.kernel.org
List-ID: <stable.vger.kernel.org>

On Fri, Nov 18, 2016 at 01:38:39PM -0500, Theodore Ts'o wrote:
> @@ -3567,7 +3567,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
>  	    blocksize > EXT4_MAX_BLOCK_SIZE) {
>  		ext4_msg(sb, KERN_ERR,
> -		       "Unsupported filesystem blocksize %d", blocksize);
> +		       "Unsupported filesystem blocksize %d (%d log_block_size)",
> +			 blocksize, le32_to_cpu(es->s_log_block_size));
> +		goto failed_mount;
> +	}
> +	if (le32_to_cpu(es->s_log_block_size) >
> +	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "Invalid log block size: %u",
> +			 le32_to_cpu(es->s_log_block_size));
>  		goto failed_mount;
>  	}
>  

This isn't validating s_log_block_size until after it's already been used in a
shift, which means the code can have undefined behavior (shift by a value too
large).  Would it make sense to do something like the following instead?
Similarly for the cluster size case.

        blocksize =                                                              
                BLOCK_SIZE << min_t(u32, le32_to_cpu(es->s_log_block_size), 20); 
        if (blocksize < EXT4_MIN_BLOCK_SIZE ||                                   
            blocksize > EXT4_MAX_BLOCK_SIZE) {                                   
                ext4_msg(sb, KERN_ERR,                                           
                        "Unsupported filesystem blocksize %d (%u bits)",         
                         blocksize, le32_to_cpu(es->s_log_block_size));          
                goto failed_mount;                                               
        }