Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [Cluster-devel] [PATCH] fs: gfs2: prevent OOB access in
 gfs2_read_sb()
To:     Andrew Price <anprice@redhat.com>
Cc:     rpeterso@redhat.com, agruenba@redhat.com, cluster-devel@redhat.com,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-kernel@vger.kernel.org,
        syzbot+a5e2482a693e6b1e444b@syzkaller.appspotmail.com,
        Fox Chen <foxhlchen@gmail.com>
References: <20201013152648.438887-1-anant.thazhemadam@gmail.com>
 <d75dc535-00be-3aa7-addb-c2615b202b56@redhat.com>
From:   Anant Thazhemadam <anant.thazhemadam@gmail.com>
Message-ID: <48a1f8ca-54ce-09f4-45c2-b1091d4e358a@gmail.com>
Date:   Wed, 14 Oct 2020 18:44:06 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <d75dc535-00be-3aa7-addb-c2615b202b56@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 14/10/20 6:34 pm, Andrew Price wrote:
> On 13/10/2020 16:26, Anant Thazhemadam wrote:
>> In gfs2_read_sb(), if the condition
>>     (d != sdp->sd_heightsize[x - 1] || m)
>> isn't satisfied (in the first 11 iterations), the loop continues,
>> and begins to perform out-of-bounds access.
>> Fix this out-of-bounds access by introducing a condition in the for loop
>> that ensures that no more than GFS2_MAX_META_HEIGHT + 1 elements are
>> accessed.
>>
>> In addition to this, if the above condition is satisfied when
>> x = GFS2_MAX_META_HEIGHT (which = 10), and the flow of control breaks
>> out of the loop, then an out-of-bounds access is performed again while
>> assigning sdp->sd_heightsize[x] = ~0 (since x would be 11 now.), and
>> also the assertion that spd->sd_max_height <= GFS2_MAX_META_HEIGHT would
>> fail.
>> Fix this out-of-bounds access and ensure that the assertion doesn't fail
>> by introducing another variable "index", which keeps track of the last
>> valid value of x (pre-increment) that can be used.
>
> That's not quite the right approach. Your analysis below is correct: the problem stems from the block size in the superblock being zeroed by the fuzzer. So the correct fix would be to add a validation check for sb_bsize (gfs2_check_sb() is lacking somewhat). Valid values are powers of 2 between 512 and the page size.
>

I see. Thanks for the review! I'll send in a v2 that implements this check soon enough.

Thanks,
Anant