|
From: Anton A. <an...@tu...> - 2022-06-24 13:53:04
|
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. So to avoid panic, remove the BUG_ON, and use ntfs_warning to output a warning to the syslog and return instead until someone really solve the problem. Cc: st...@vg... Reported-by: Zeal Robot <ze...@zt...> Reported-by: syz...@sy... Reviewed-by: Songyi Zhang <zha...@zt...> Reviewed-by: Yang Yang <yan...@zt...> Reviewed-by: Jiang Xuexin<jia...@zt...> Reviewed-by: Zhang wenya<zha...@zt...> Signed-off-by: xu xin <xu....@zt...> --- Change for v2: - Use ntfs_warning instead of WARN(). - Add the tag Cc: st...@vg.... --- fs/ntfs/aops.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c index 5f4fb6ca6f2e..84d68efb4ace 100644 --- a/fs/ntfs/aops.c +++ b/fs/ntfs/aops.c @@ -183,7 +183,12 @@ 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 (unlikely(!ni->runlist.rl && !ni->mft_no && !NInoAttr(ni))) { + ntfs_warning(vi->i_sb, "Error because ni->runlist.rl, ni->mft_no, " + "and NInoAttr(ni) is null."); + unlock_page(page); + return -EINVAL; + } A better warning message that doesn't rely on implementation details (struct field and macro names) would be "Runlist of $MFT/$DATA is not cached". Also, why does this situation happen in the first place? Is there a way to prevent this situation in the first place? ntfs_mapping_pairs_decompress() should return error pointer instead of NULL. Callers is checking error value using IS_ERR(). and the mapping pairs array of @MFT entry is empty, I think it's corrupted, it should cause mount failure. NAK Sorry but this patch is incorrect. It is perfectly valid to have an empty non-resident attribute. E.g. if you truncate a file to zero size this is exactly what you will get on-disk and when you then unmount and mount next time and try to access that file with your patch you will now get an -EIO error trying to access the file and you will not be able to write to the file nor truncate it as you will keep getting the i/o error. 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... Best regards, Anton I haven't checked if this patch fix the problem. Xu, Can you check it ? diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c index 97932fb5179c..31263fe0772f 100644 --- a/fs/ntfs/runlist.c +++ 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 |
