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;
Date:   Mon, 5 Jul 2021 13:33:29 +0200
From:   Jan Kara <jack@suse.cz>
To:     Shreyansh Chouhan <chouhan.shreyansh630@gmail.com>
Cc:     jack@suse.cz, rkovhaev@gmail.com, reiserfs-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: Verify the items that we read from blocks
Message-ID: <20210705113329.GE15373@quack2.suse.cz>
References: <YN8rRYxhZvAa+VxU@fedora>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YN8rRYxhZvAa+VxU@fedora>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hello!

On Fri 02-07-21 20:35:41, Shreyansh Chouhan wrote:
> I was trying to work on this[1] bug. After a lot of reading the code and
> running it under gdb, I found out that the error happens because
> syzkaller creates a segment with raw binary data in the reproducer[2],
> that has the wrong deh_location for the `..` directory item. (The value
> is 0x5d (93), where as it should have been 0x20 (32).)

First, I'd like to note that reiserfs is a legacy filesystem which gets
little maintenance and I think distributions are close to disabling it in
their default kernels if they didn't do it already. So I'm not sure how
much is it worth it to do any larger fixes to it. But if you have a
personal passion for reiserfs feel free to go ahead and try to fix these
issues.

> I think that the solution would involve checking the items that we read,
> and verify that they are actually valid. But this check could actually
> happen in two places:
> 
> - First idea would be to check as soon as we read a
>   block, and one way of doing that would be adding a wrapper around
>   ll_rw_block that validates the leaf node blocks that we read. The
>   benifits to this would be that since we're solving the problem at it's
>   root, very few functions would have to be changed. But I don't know
>   how much of a performance hit would it be.

It depends on how heavy the checks are going to be but generally checking
when loading from the disk is the way how most filesystems handle this.

> - Second idea would be to do these validation checks lazily. This should
>   be faster than the first idea, but this would involve changing the
>   code at more places than in the first idea.
> 
> For how the validation happens, the first idea that comes to mind is
> reading the item headers from the block that we read and verifying if
> the header is valid, and if the items themselves are valid according to
> the header.

Looks sound.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR