Re: [Linux-NTFS-Dev] [PATCH v2] fs/ntfs: fix BUG_ON of ntfs_read_block()

[CGEL <cge...@gm...>]

On Fri, Jun 24, 2022 at 03:26:27PM +0000, Anton Altaparmakov wrote:
> Hi,
> 
> > On 24 Jun 2022, at 15:37, Namjae Jeon <lin...@ke...> wrote:
> > 2022-06-24 21:19 GMT+09:00, Anton Altaparmakov <an...@tu...>:
> >> Hi,
> >> 
> >> On 24 Jun 2022, at 03:33, Namjae Jeon
> >> <lin...@ke...<mailto:lin...@ke...>> wrote:
> >> 
> >> 2022-06-24 2:08 GMT+09:00, Eric Biggers
> >> <ebi...@ke...<mailto:ebi...@ke...>>:
> >> On Thu, Jun 23, 2022 at 09:49:56AM +0000,
> >> cge...@gm...<mailto:cge...@gm...> wrote:
> >> From: xu xin <xu....@zt...<mailto:xu....@zt...>>
> >> 
> >> As the bug description at
> >> https://lore.kernel.org/lkml/202...@zt.../
> >> attckers can use this bug to crash the system.
> >> 
> >> 
> 
> >> The correct solution is to use IS_ERR_OR_NULL() in places where an empty
> >> 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 may be
> >> more call sites to ntfs_mapping_pairs_decompress() which require similar
> >> treatment. Need to go through the code to see...
> > I think that it is needed everywhere that calls it. Am I missing something ?
> 
> It is not needed everywhere.  It is only needed in code paths that require 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 runlist as we are already reading from the $MFT so it has an allocation so the runlist cannot be empty.  But the fuzzed image that is showing the problem that you are trying to fix has such a corrupt volume that the runlist is in fact empty - note it is NOT corrupt but it is valid but empty.  This then leads to the problem that we try to load the runlist for the $MFT and we do not 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 happens to be empty then we want a NULL runlist to be obtained so we can then 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 attributes before working with them and if the attribute is zero length then the runlist 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 = ni->vol;
      
           /* $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

> 
> 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 attributes 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 whether an attribute has zero allocated_size (or compressed_size if compressed and/or sparse) and if so you would need to then not attempt to map the runlist which would be ugly.  Much better to have the code path streamline so it just returns an empty runlist...
> 
> It may be possible that the current OSS ntfs driver can indeed function with your patch but it still violates how the code is designed to work which is why I am not happy to take your patch.
> 
> > I can not understand why the below code is needed in
> > ntfs_mapping_pairs_decompress().
> > 
> > /* If the mapping pairs array is valid but empty, nothing to do. */
> > if (!vcn && !*buf) {
> > return old_rl;
> > }
> > 
> >> +++ 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 = 0;
> >> /* Allocate first page and set current runlist size to one page. */
> >> 
> >> 
> >> - Eric