Corrected validation of multi sector transfer protected records

The validation contained an off-by-one error.  The
expression '(u32)(usa_ofs + (usa_count * 2)) > size' used 'usa_count'
after it had been decremented to skip the update sequence number entry.
Consequently, the code could read out of bounds, up to two bytes past the
end of the MST-protected record.

Furthermore, as documented in the comment in layout.h for "NTFS_RECORD"
and also on MSDN for "MULTI_SECTOR_HEADER", the update sequence array
must end before the last le16 in the first logical sector --- not merely
before the end of the record.

Fix the validation and move it into a helper function, as it was done
identically in the read and write paths.

Signed-off-by: Eric Biggers <ebiggers3@gmail.com>

include/ntfs-3g/layout.h

@@ -161,6 +161,12 @@ typedef enum {
 #define ntfs_is_empty_recordp(p)	( ntfs_is_magicp(p, empty) )
 
 
+/*
+ * The size of a logical sector in bytes, used as the sequence number stride for
+ * multi-sector transfers.  This is intended to be less than or equal to the
+ * physical sector size, since if this were greater than the physical sector
+ * size, then incomplete multi-sector transfers may not be detected.
+ */
 #define NTFS_BLOCK_SIZE		512
 #define NTFS_BLOCK_SIZE_BITS	9
 

libntfs-3g/mst.c

@@ -31,6 +31,21 @@
 #include "mst.h"
 #include "logging.h"
 
+/*
+ * Basic validation of a NTFS multi-sector record.  The record size must be a
+ * multiple of the logical sector size; and the update sequence array must be
+ * properly aligned, of the expected length, and must end before the last le16
+ * in the first logical sector.
+ */
+static BOOL
+is_valid_record(u32 size, u16 usa_ofs, u16 usa_count)
+{
+	return size % NTFS_BLOCK_SIZE == 0 &&
+		usa_ofs % 2 == 0 &&
+		usa_count == 1 + (size / NTFS_BLOCK_SIZE) &&
+		usa_ofs + ((u32)usa_count * 2) <= NTFS_BLOCK_SIZE - 2;
+}
+
 /**
  * ntfs_mst_post_read_fixup - deprotect multi sector transfer protected data
  * @b:		pointer to the data to deprotect
@@ -57,12 +72,9 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const u32 size,
 
 	/* Setup the variables. */
 	usa_ofs = le16_to_cpu(b->usa_ofs);
-	/* Decrement usa_count to get number of fixups. */
+	usa_count = le16_to_cpu(b->usa_count);
-	usa_count = le16_to_cpu(b->usa_count) - 1;
+
-	/* Size and alignment checks. */
+	if (!is_valid_record(size, usa_ofs, usa_count)) {
-	if (size & (NTFS_BLOCK_SIZE - 1) || usa_ofs & 1 ||
-			(u32)(usa_ofs + (usa_count * 2)) > size ||
-			(size >> NTFS_BLOCK_SIZE_BITS) != usa_count) {
 		errno = EINVAL;
 		if (warn) {
 			ntfs_log_perror("%s: magic: 0x%08lx  size: %ld "
@@ -91,7 +103,7 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const u32 size,
 	/*
 	 * Check for incomplete multi sector transfer(s).
 	 */
-	while (usa_count--) {
+	while (--usa_count) {
 		if (*data_pos != usn) {
 			/*
 			 * Incomplete multi sector transfer detected! )-:
@@ -109,10 +121,10 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const u32 size,
 		data_pos += NTFS_BLOCK_SIZE/sizeof(u16);
 	}
 	/* Re-setup the variables. */
-	usa_count = le16_to_cpu(b->usa_count) - 1;
+	usa_count = le16_to_cpu(b->usa_count);
 	data_pos = (u16*)b + NTFS_BLOCK_SIZE/sizeof(u16) - 1;
 	/* Fixup all sectors. */
-	while (usa_count--) {
+	while (--usa_count) {
 		/*
 		 * Increment position in usa and restore original data from
 		 * the usa into the data buffer.
@@ -171,12 +183,9 @@ int ntfs_mst_pre_write_fixup(NTFS_RECORD *b, const u32 size)
 	}
 	/* Setup the variables. */
 	usa_ofs = le16_to_cpu(b->usa_ofs);
-	/* Decrement usa_count to get number of fixups. */
+	usa_count = le16_to_cpu(b->usa_count);
-	usa_count = le16_to_cpu(b->usa_count) - 1;
+
-	/* Size and alignment checks. */
+	if (!is_valid_record(size, usa_ofs, usa_count)) {
-	if (size & (NTFS_BLOCK_SIZE - 1) || usa_ofs & 1 ||
-			(u32)(usa_ofs + (usa_count * 2)) > size ||
-			(size >> NTFS_BLOCK_SIZE_BITS) != usa_count) {
 		errno = EINVAL;
 		ntfs_log_perror("%s", __FUNCTION__);
 		return -1;
@@ -195,7 +204,7 @@ int ntfs_mst_pre_write_fixup(NTFS_RECORD *b, const u32 size)
 	/* Position in data of first le16 that needs fixing up. */
 	data_pos = (le16*)b + NTFS_BLOCK_SIZE/sizeof(le16) - 1;
 	/* Fixup all sectors. */
-	while (usa_count--) {
+	while (--usa_count) {
 		/*
 		 * Increment the position in the usa and save the
 		 * original data from the data buffer into the usa.
@@ -223,7 +232,7 @@ void ntfs_mst_post_write_fixup(NTFS_RECORD *b)
 	u16 *usa_pos, *data_pos;
 
 	u16 usa_ofs = le16_to_cpu(b->usa_ofs);
-	u16 usa_count = le16_to_cpu(b->usa_count) - 1;
+	u16 usa_count = le16_to_cpu(b->usa_count);
 
 	ntfs_log_trace("Entering\n");
 
@@ -234,7 +243,7 @@ void ntfs_mst_post_write_fixup(NTFS_RECORD *b)
 	data_pos = (u16*)b + NTFS_BLOCK_SIZE/sizeof(u16) - 1;
 
 	/* Fixup all sectors. */
-	while (usa_count--) {
+	while (--usa_count) {
 		/*
 		 * Increment position in usa and restore original data from
 		 * the usa into the data buffer.