[PATCH] ntfs: add check for mft record size in superblock

[PATCH] ntfs: add check for mft record size in superblock

[Rustam Kovhaev]

number of bytes allocated for mft record should be equal to the mft
record size stored in ntfs superblock
as reported by syzbot, userspace might trigger out-of-bounds read by
dereferencing ctx->attr in ntfs_attr_find()

Reported-and-tested-by: syzbot+aed06913f36eff9b544e@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=aed06913f36eff9b544e
Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
Acked-by: Anton Altaparmakov <anton@tuxera.com>
---
 fs/ntfs/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 9bb9f0952b18..caf563981532 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1810,6 +1810,12 @@ int ntfs_read_inode_mount(struct inode *vi)
 		brelse(bh);
 	}
 
+	if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
+		ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
+				le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
+		goto err_out;
+	}
+
 	/* Apply the mst fixups. */
 	if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
 		/* FIXME: Try to use the $MFTMirr now. */
-- 
2.28.0

Re: [PATCH] ntfs: add check for mft record size in superblock

[Anton Altaparmakov]

Hi Andrew,

Can you please merge this patch?  Thanks a lot in advance!

Rustam, thank you for the updated patch!

Best regards,

	Anton

> On 24 Aug 2020, at 03:28, Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> 
> number of bytes allocated for mft record should be equal to the mft
> record size stored in ntfs superblock
> as reported by syzbot, userspace might trigger out-of-bounds read by
> dereferencing ctx->attr in ntfs_attr_find()
> 
> Reported-and-tested-by: syzbot+aed06913f36eff9b544e@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=aed06913f36eff9b544e
> Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
> Acked-by: Anton Altaparmakov <anton@tuxera.com>
> ---
> fs/ntfs/inode.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 9bb9f0952b18..caf563981532 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1810,6 +1810,12 @@ int ntfs_read_inode_mount(struct inode *vi)
> 		brelse(bh);
> 	}
> 
> +	if (le32_to_cpu(m->bytes_allocated) != vol->mft_record_size) {
> +		ntfs_error(sb, "Incorrect mft record size %u in superblock, should be %u.",
> +				le32_to_cpu(m->bytes_allocated), vol->mft_record_size);
> +		goto err_out;
> +	}
> +
> 	/* Apply the mst fixups. */
> 	if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
> 		/* FIXME: Try to use the $MFTMirr now. */
> -- 
> 2.28.0
> 


-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

[PATCH] ntfs: add check for mft record size in superblock

[Rustam Kovhaev]

number of bytes allocated for mft record should be equal to the mft
record size stored in ntfs superblock
as reported by syzbot, userspace might trigger out-of-bounds read by
dereferencing ctx->attr in ntfs_attr_find()

Reported-and-tested-by: syzbot+aed06913f36eff9b544e@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=aed06913f36eff9b544e
Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
---
 fs/ntfs/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 9bb9f0952b18..6407af7c2e4f 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1810,6 +1810,12 @@ int ntfs_read_inode_mount(struct inode *vi)
 		brelse(bh);
 	}
 
+	if (m->bytes_allocated != vol->mft_record_size) {
+		ntfs_error(sb, "Incorrect mft record size [%u] in superblock, should be [%u].",
+				m->bytes_allocated, vol->mft_record_size);
+		goto err_out;
+	}
+
 	/* Apply the mst fixups. */
 	if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
 		/* FIXME: Try to use the $MFTMirr now. */
-- 
2.28.0

Re: [PATCH] ntfs: add check for mft record size in superblock

[Anton Altaparmakov]

Hi Rustam,

Thank you for the patch but it introduces an endianness bug - you have to us le32_to_cpu(m->bytes_allocated) both when doing the comparison and then printing the message.

Also, please drop the square brackets.  Wherever the driver prints such things it never uses brackets around the numbers and it would be better to have this consistent throughout.

Can you please resend with the above issues addressed?  You can then also add to the commit message:

	Acked-by: Anton Altaparmakov <anton@tuxera.com>

Thanks!

Best regards,

	Anton

> On 23 Aug 2020, at 16:21, Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> 
> number of bytes allocated for mft record should be equal to the mft
> record size stored in ntfs superblock
> as reported by syzbot, userspace might trigger out-of-bounds read by
> dereferencing ctx->attr in ntfs_attr_find()
> 
> Reported-and-tested-by: syzbot+aed06913f36eff9b544e@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=aed06913f36eff9b544e
> Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com>
> ---
> fs/ntfs/inode.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 9bb9f0952b18..6407af7c2e4f 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1810,6 +1810,12 @@ int ntfs_read_inode_mount(struct inode *vi)
> 		brelse(bh);
> 	}
> 
> +	if (m->bytes_allocated != vol->mft_record_size) {
> +		ntfs_error(sb, "Incorrect mft record size [%u] in superblock, should be [%u].",
> +				m->bytes_allocated, vol->mft_record_size);
> +		goto err_out;
> +	}
> +
> 	/* Apply the mst fixups. */
> 	if (post_read_mst_fixup((NTFS_RECORD*)m, vol->mft_record_size)) {
> 		/* FIXME: Try to use the $MFTMirr now. */
> -- 
> 2.28.0
> 


-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Re: [PATCH] ntfs: add check for mft record size in superblock

[Rustam Kovhaev]

On Mon, Aug 24, 2020 at 01:44:06AM +0000, Anton Altaparmakov wrote:
> Hi Rustam,
> 
> Thank you for the patch but it introduces an endianness bug - you have to us le32_to_cpu(m->bytes_allocated) both when doing the comparison and then printing the message.
> 
> Also, please drop the square brackets.  Wherever the driver prints such things it never uses brackets around the numbers and it would be better to have this consistent throughout.
> 
> Can you please resend with the above issues addressed?  You can then also add to the commit message:
> 
> 	Acked-by: Anton Altaparmakov <anton@tuxera.com>
> 
> Thanks!
> 
> Best regards,
> 
> 	Anton
> 
hi Anton,
thank you for the review, my bad, i'll get it fixed and i'll resend the patch