[PATCH 1/2] fs/ntfs3: map the right page of large folios in ntfs_read_write_run()

[PATCH 1/2] fs/ntfs3: map the right page of large folios in ntfs_read_write_run()

[Senjin]

ntfs_read_write_run() reads and writes through the block device page
cache. It looks up the folio that contains lbo with read_mapping_folio(),
but then maps offset 0 of that folio and only adds offset_in_page(lbo):

	folio = read_mapping_folio(mapping, lbo >> PAGE_SHIFT, NULL);
	off = offset_in_page(lbo);
	kaddr = kmap_local_folio(folio, 0);
	memcpy(buf, kaddr + off, op);

This is only correct for order-0 folios. If the folio is a large folio
that starts before lbo, the data is copied from (or written to) the wrong
page, (index - folio->index) * PAGE_SIZE bytes before the intended
position.

ntfs3 itself populates the block device mapping with order-0 folios, so
this goes unnoticed on a cold cache. But a userspace read of the block
device (ntfs-3g tools such as ntfscat/ntfsinfo, probing tools, ...) fills
the same mapping through readahead with large folios. After that,
reading a WOF (LZX/XPRESS) compressed file passes garbage to the
decompressor: on a test volume, lzx_decompress() failed for 33 of 35
frames of a 1.1 MB file, and the compressed input it was given matched
the real WofCompressedData stream shifted back by 4 KiB to 52 KiB.

Reproducer (WOF-compressed file, e.g. made with "compact /c /exe:lzx"):

	echo 3 > /proc/sys/vm/drop_caches
	ntfscat -f -a 0x80 -n WofCompressedData $DEV /path/file >/dev/null
	md5sum /mnt/path/file    # wrong; correct without the ntfscat step

On a corpus of 16,716 WOF-compressed files, reading each file right
after ntfscat had read its stream gave wrong contents for 1,051 of them,
nearly all files spanning more than a few 32 KiB frames. With this patch
all 16,716 read correctly, and plain reads with a cold cache were
correct both before and after.

The same helper is used for writes (ni_write_frame(), and
ni_decompress_file() via ni_read_frame()), so on a read-write mount the
data could be written to the wrong place on disk as well.

Map the page that contains lbo. op is already limited to the end of that
page, so a single kmap_local_folio() mapping is enough.

Fixes: f35590ee26f5 ("fs/ntfs3: remove ntfs_bio_pages and use page cache for compressed I/O")
Cc: stable@vger.kernel.org # 6.19+
Signed-off-by: Senjin <senjin@hatchling.org>
---
 fs/ntfs3/fsntfs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index 97c04ab..3340e72 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -1557,12 +1557,17 @@ int ntfs_read_write_run(struct ntfs_sb_info *sbi, const struct runs_tree *run,
 		if (op > bytes)
 			op = bytes;
 
-		kaddr = kmap_local_folio(folio, 0);
+		/*
+		 * The folio may be a large folio that starts before
+		 * lbo (e.g. created by readahead on the block device),
+		 * so map the page that actually contains lbo.
+		 */
+		kaddr = kmap_local_folio(folio, offset_in_folio(folio, lbo));
 		if (wr) {
-			memcpy(kaddr + off, buf, op);
+			memcpy(kaddr, buf, op);
 			folio_mark_dirty(folio);
 		} else {
-			memcpy(buf, kaddr + off, op);
+			memcpy(buf, kaddr, op);
 			flush_dcache_folio(folio);
 		}
 		kunmap_local(kaddr);
-- 
2.56.0

[PATCH 1/2] fs/ntfs3: map the right page of large folios in ntfs_read_write_run()

[Senjin]

ntfs_read_write_run() reads and writes through the block device page
cache. It looks up the folio that contains lbo with read_mapping_folio(),
but then maps offset 0 of that folio and only adds offset_in_page(lbo):

	folio = read_mapping_folio(mapping, lbo >> PAGE_SHIFT, NULL);
	off = offset_in_page(lbo);
	kaddr = kmap_local_folio(folio, 0);
	memcpy(buf, kaddr + off, op);

This is only correct for order-0 folios. If the folio is a large folio
that starts before lbo, the data is copied from (or written to) the wrong
page, (index - folio->index) * PAGE_SIZE bytes before the intended
position.

ntfs3 itself populates the block device mapping with order-0 folios, so
this goes unnoticed on a cold cache. But a userspace read of the block
device (ntfs-3g tools such as ntfscat/ntfsinfo, probing tools, ...) fills
the same mapping through readahead with large folios. After that,
reading a WOF (LZX/XPRESS) compressed file passes garbage to the
decompressor: on a test volume, lzx_decompress() failed for 33 of 35
frames of a 1.1 MB file, and the compressed input it was given matched
the real WofCompressedData stream shifted back by 4 KiB to 52 KiB.

Reproducer (WOF-compressed file, e.g. made with "compact /c /exe:lzx"):

	echo 3 > /proc/sys/vm/drop_caches
	ntfscat -f -a 0x80 -n WofCompressedData $DEV /path/file >/dev/null
	md5sum /mnt/path/file    # wrong; correct without the ntfscat step

On a corpus of 16,716 WOF-compressed files, reading each file right
after ntfscat had read its stream gave wrong contents for 1,051 of them,
nearly all files spanning more than a few 32 KiB frames. With this patch
all 16,716 read correctly, and plain reads with a cold cache were
correct both before and after.

The same helper is used for writes (ni_write_frame(), and
ni_decompress_file() via ni_read_frame()), so on a read-write mount the
data could be written to the wrong place on disk as well.

Map the page that contains lbo. op is already limited to the end of that
page, so a single kmap_local_folio() mapping is enough.

Fixes: f35590ee26f5 ("fs/ntfs3: remove ntfs_bio_pages and use page cache for compressed I/O")
Cc: stable@vger.kernel.org # 6.19+
Signed-off-by: Senjin <senjin@hatchling.org>
---
 fs/ntfs3/fsntfs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index 97c04ab..3340e72 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -1557,12 +1557,17 @@ int ntfs_read_write_run(struct ntfs_sb_info *sbi, const struct runs_tree *run,
 		if (op > bytes)
 			op = bytes;
 
-		kaddr = kmap_local_folio(folio, 0);
+		/*
+		 * The folio may be a large folio that starts before
+		 * lbo (e.g. created by readahead on the block device),
+		 * so map the page that actually contains lbo.
+		 */
+		kaddr = kmap_local_folio(folio, offset_in_folio(folio, lbo));
 		if (wr) {
-			memcpy(kaddr + off, buf, op);
+			memcpy(kaddr, buf, op);
 			folio_mark_dirty(folio);
 		} else {
-			memcpy(buf, kaddr + off, op);
+			memcpy(buf, kaddr, op);
 			flush_dcache_folio(folio);
 		}
 		kunmap_local(kaddr);
-- 
2.56.0

[PATCH 2/2] fs/ntfs3: don't mark pages uptodate when ni_read_frame() fails

[Senjin]

ni_read_frame() calls SetPageUptodate() on every page of the frame on
exit, including when reading or decompressing the frame failed. The
pages then hold zeroes or partially decompressed data, and that is
served as valid file data until the page cache is dropped.

The error is also lost for the folio that was actually requested:
ni_read_folio_cmpr() returns it from ->read_folio, but
filemap_read_folio() only checks folio_test_uptodate() afterwards and
treats the read as successful. Userspace gets wrong data, read() returns
no error, and nothing is logged.

Only mark the pages uptodate when the frame was read successfully. On
failure the requested folio is left !uptodate and the read returns an
error; the other pages of the frame are retried on their next access.

With the bug fixed by the previous patch still present, a read of an
affected file now fails with an error after the last good frame, instead
of silently returning zeroes.

Fixes: f35590ee26f5 ("fs/ntfs3: remove ntfs_bio_pages and use page cache for compressed I/O")
Cc: stable@vger.kernel.org # 6.19+
Signed-off-by: Senjin <senjin@hatchling.org>
---
 fs/ntfs3/frecord.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bead01a..a8b696d 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2287,7 +2287,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 	u64 vbo_disk;
 	size_t unc_size = 0;
 	u32 frame_size, i, ondisk_size;
-	struct page *pg;
 	struct ATTRIB *attr;
 	CLST frame, clst_data;
 
@@ -2519,9 +2518,14 @@ out1:
 #endif
 	vunmap(frame_mem);
 out:
-	for (i = 0; i < pages_per_frame; i++) {
-		pg = pages[i];
-		SetPageUptodate(pg);
+	/*
+	 * Only mark the pages uptodate if the frame was read successfully,
+	 * otherwise zeroed or partially decompressed pages would be served
+	 * from the page cache as valid data.
+	 */
+	if (!err) {
+		for (i = 0; i < pages_per_frame; i++)
+			SetPageUptodate(pages[i]);
 	}
 
 	return err;
-- 
2.56.0

[PATCH 2/2] fs/ntfs3: don't mark pages uptodate when ni_read_frame() fails

[Senjin]

ni_read_frame() calls SetPageUptodate() on every page of the frame on
exit, including when reading or decompressing the frame failed. The
pages then hold zeroes or partially decompressed data, and that is
served as valid file data until the page cache is dropped.

The error is also lost for the folio that was actually requested:
ni_read_folio_cmpr() returns it from ->read_folio, but
filemap_read_folio() only checks folio_test_uptodate() afterwards and
treats the read as successful. Userspace gets wrong data, read() returns
no error, and nothing is logged.

Only mark the pages uptodate when the frame was read successfully. On
failure the requested folio is left !uptodate and the read returns an
error; the other pages of the frame are retried on their next access.

With the bug fixed by the previous patch still present, a read of an
affected file now fails with an error after the last good frame, instead
of silently returning zeroes.

Fixes: f35590ee26f5 ("fs/ntfs3: remove ntfs_bio_pages and use page cache for compressed I/O")
Cc: stable@vger.kernel.org # 6.19+
Signed-off-by: Senjin <senjin@hatchling.org>
---
 fs/ntfs3/frecord.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index bead01a..a8b696d 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -2287,7 +2287,6 @@ int ni_read_frame(struct ntfs_inode *ni, u64 frame_vbo, struct page **pages,
 	u64 vbo_disk;
 	size_t unc_size = 0;
 	u32 frame_size, i, ondisk_size;
-	struct page *pg;
 	struct ATTRIB *attr;
 	CLST frame, clst_data;
 
@@ -2519,9 +2518,14 @@ out1:
 #endif
 	vunmap(frame_mem);
 out:
-	for (i = 0; i < pages_per_frame; i++) {
-		pg = pages[i];
-		SetPageUptodate(pg);
+	/*
+	 * Only mark the pages uptodate if the frame was read successfully,
+	 * otherwise zeroed or partially decompressed pages would be served
+	 * from the page cache as valid data.
+	 */
+	if (!err) {
+		for (i = 0; i < pages_per_frame; i++)
+			SetPageUptodate(pages[i]);
 	}
 
 	return err;
-- 
2.56.0