Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <62c3ec88.1c69fb81.b5648.7f2b@mx.google.com>
Date:   Tue, 5 Jul 2022 07:47:18 +0000
From:   CGEL <cgel.zte@gmail.com>
To:     Anton Altaparmakov <anton@tuxera.com>
Cc:     Namjae Jeon <linkinjeon@kernel.org>,
        Eric Biggers <ebiggers@kernel.org>,
        "xu.xin16@zte.com.cn" <xu.xin16@zte.com.cn>,
        "linux-ntfs-dev@lists.sourceforge.net" 
        <linux-ntfs-dev@lists.sourceforge.net>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Zeal Robot <zealci@zte.com.cn>,
        "syzbot+6a5a7672f663cce8b156@syzkaller.appspotmail.com" 
        <syzbot+6a5a7672f663cce8b156@syzkaller.appspotmail.com>,
        Songyi Zhang <zhang.songyi@zte.com.cn>,
        Yang Yang <yang.yang29@zte.com.cn>,
        Jiang Xuexin <jiang.xuexin@zte.com.cn>,
        Zhang wenya <zhang.wenya1@zte.com.cn>
Subject: Re: [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()
References: <20220623033635.973929-1-xu.xin16@zte.com.cn>
 <20220623094956.977053-1-xu.xin16@zte.com.cn>
 <YrSeAGmk4GZndtdn@sol.localdomain>
 <CAKYAXd8qoLKbWGX7omYUfarSugRnose8X8o3Zhb1XctiUtamQg@mail.gmail.com>
 <7FBC6FD2-5D60-4EB8-96D5-A6014D271950@tuxera.com>
 <CAKYAXd-cg5rvXo-MYZ29+wiE3BYbg4vqDHrOHtu77ox-+7dPBw@mail.gmail.com>
 <790E333B-FFA5-4C8E-95D3-BCBA7679F9C4@tuxera.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <790E333B-FFA5-4C8E-95D3-BCBA7679F9C4@tuxera.com>
Precedence: bulk

On Fri, Jun 24, 2022 at 03:26:27PM +0000, Anton Altaparmakov wrote:
> Hi,
>=20
> > On 24 Jun 2022, at 15:37, Namjae Jeon <linkinjeon@kernel.org> wrote:
> > 2022-06-24 21:19 GMT+09:00, Anton Altaparmakov <anton@tuxera.com>:
> >> Hi,
> >>=20
> >> On 24 Jun 2022, at 03:33, Namjae Jeon
> >> <linkinjeon@kernel.org<mailto:linkinjeon@kernel.org>> wrote:
> >>=20
> >> 2022-06-24 2:08 GMT+09:00, Eric Biggers
> >> <ebiggers@kernel.org<mailto:ebiggers@kernel.org>>:
> >> On Thu, Jun 23, 2022 at 09:49:56AM +0000,
> >> cgel.zte@gmail.com<mailto:cgel.zte@gmail.com> wrote:
> >> From: xu xin <xu.xin16@zte.com.cn<mailto:xu.xin16@zte.com.cn>>
> >>=20
> >> As the bug description at
> >> https://lore.kernel.org/lkml/20220623033635.973929-1-xu.xin16@zte.com.=
cn/
> >> attckers can use this bug to crash the system.
> >>=20
> >>=20
>=20
> >> The correct solution is to use IS_ERR_OR_NULL() in places where an emp=
ty
> >> attribute is not acceptable. Such a case is for example when mounting =
the
> >> $MFT::$DATA::unnamed attribute cannot be empty which should then be
> >> addressed inside in fs/ntfs/inode.c::ntfs_read_inode_mount(). There ma=
y be
> >> more call sites to ntfs_mapping_pairs_decompress() which require simil=
ar
> >> treatment. Need to go through the code to see...
> > I think that it is needed everywhere that calls it. Am I missing someth=
ing ?
>=20
> It is not needed everywhere.  It is only needed in code paths that requir=
e there be a runlist afterwards because the code paths assume there must be=
 one.  So for example for $MFT it by definition cannot have an empty runlis=
t as we are already reading from the $MFT so it has an allocation so the ru=
nlist cannot be empty.  But the fuzzed image that is showing the problem th=
at you are trying to fix has such a corrupt volume that the runlist is in f=
act empty - note it is NOT corrupt but it is valid but empty.  This then le=
ads to the problem that we try to load the runlist for the $MFT and we do n=
ot check whether the result is empty because we assume it cannot be empty. =
 Clearly when you have a corrupted image that assumption can be wrong so we=
 need to check for both error and for it being empty, hence IS_ERR_OR_NULL(=
).  But if we are accessing an attribute elsewhere in the driver and that h=
appens to be empty then we want a NULL runlist to be obtained so we can the=
n operate with the attribute.  We do not want attempting to map the runlist=
 to fail.  The code can try to map the runlist for all non-resident attribu=
tes before working with them and if the attribute is zero length then the r=
unlist is NULL until we write something to the attribute at which point it =
becomes non-NULL describing the allocated clusters.


So to fix the current bug, how about this patch?

--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -183,7 +183,14 @@ static int ntfs_read_block(struct page *page)
     vol =3D ni->vol;
     =20
           /* $MFT/$DATA must have its complete runlist in memory at all
	    * times. */
	    -    BUG_ON(!ni->runlist.rl && !ni->mft_no &&
		 !NInoAttr(ni));
		 +    if (IS_ERR_OR_NULL(ni->runlist.rl) && !ni->mft_no
		 && !NInoAttr(ni)) {
		 +        ntfs_error(vol->sb, "Runlist of $MFT/$DATA is not cached. "
		 +                    "$MFT is corrupt.");
		 +        unlock_page(page);
		 +        if (IS_ERR(ni->runlist.rl))
		 +            return PTR_ERR(ni->runlist.rl);
		 +        return -EFAULT;
		 +    }
--

Thanks
xu xin

>=20
> Your patch violates the design of the code which is that empty attribute =
inodes have a NULL runlist and your patch will cause perfectly good attribu=
tes to be rejected if their runlist is attempted to be mapped when they are=
 empty.  You would then have to start checking everywhere in the code wheth=
er an attribute has zero allocated_size (or compressed_size if compressed a=
nd/or sparse) and if so you would need to then not attempt to map the runli=
st which would be ugly.  Much better to have the code path streamline so it=
 just returns an empty runlist...
>=20
> It may be possible that the current OSS ntfs driver can indeed function w=
ith your patch but it still violates how the code is designed to work which=
 is why I am not happy to take your patch.
>=20
> > I can not understand why the below code is needed in
> > ntfs_mapping_pairs_decompress().
> >=20
> > /* If the mapping pairs array is valid but empty, nothing to do. */
> > if (!vcn && !*buf) {
> > return old_rl;
> > }
> >=20
> >> +++ b/fs/ntfs/runlist.c
> >> @@ -766,8 +766,11 @@ runlist_element
> >> *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
> >> return ERR_PTR(-EIO);
> >> }
> >> /* If the mapping pairs array is valid but empty, nothing to do. */
> >> - if (!vcn && !*buf)
> >> + if (!vcn && !*buf) {
> >> + if (!old_rl)
> >> + return ERR_PTR(-EIO);
> >> return old_rl;
> >> + }
> >> /* Current position in runlist array. */
> >> rlpos =3D 0;
> >> /* Allocate first page and set current runlist size to one page. */
> >>=20
> >>=20
> >> - Eric