[PATCH] ntfs3: Add bounds checking to mi_enum_attr()

[PATCH] ntfs3: Add bounds checking to mi_enum_attr()

[lei lu]

Added bounds checking to make sure that every attr don't stray beyond valid
memory region.

Signed-off-by: lei lu <llfamsec@gmail.com>
---
 fs/ntfs3/record.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
index 6aa3a9d44df1..d47ca8ca270b 100644
--- a/fs/ntfs3/record.c
+++ b/fs/ntfs3/record.c
@@ -223,34 +223,35 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
 		prev_type = 0;
 		attr = Add2Ptr(rec, off);
 	} else {
-		/* Check if input attr inside record. */
+		/*
+		 * We don't need to check previous attr here. There is
+		 * a bounds checking in the previous round.
+		 */
 		off = PtrOffset(rec, attr);
-		if (off >= used)
-			return NULL;
 
 		asize = le32_to_cpu(attr->size);
-		if (asize < SIZEOF_RESIDENT) {
-			/* Impossible 'cause we should not return such attribute. */
-			return NULL;
-		}
-
-		/* Overflow check. */
-		if (off + asize < off)
-			return NULL;
 
 		prev_type = le32_to_cpu(attr->type);
 		attr = Add2Ptr(attr, asize);
 		off += asize;
 	}
 
-	asize = le32_to_cpu(attr->size);
-
 	/* Can we use the first field (attr->type). */
 	if (off + 8 > used) {
 		static_assert(ALIGN(sizeof(enum ATTR_TYPE), 8) == 8);
 		return NULL;
 	}
 
+	asize = le32_to_cpu(attr->size);
+	if (asize < SIZEOF_RESIDENT) {
+		/* Impossible 'cause we should not return such attribute. */
+		return NULL;
+	}
+
+	/* Check overflow and boundary. */
+	if (off + asize < off || off + asize > used)
+		return NULL;
+
 	if (attr->type == ATTR_END) {
 		/* End of enumeration. */
 		return NULL;
@@ -265,10 +266,6 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
 	if (t32 < prev_type)
 		return NULL;
 
-	/* Check overflow and boundary. */
-	if (off + asize < off || off + asize > used)
-		return NULL;
-
 	/* Check size of attribute. */
 	if (!attr->non_res) {
 		/* Check resident fields. */
-- 
2.34.1

[PATCH v2] ntfs3: Add bounds checking to mi_enum_attr()

[lei lu]

Added bounds checking to make sure that every attr don't stray beyond
valid memory region.

Signed-off-by: lei lu <llfamsec@gmail.com>
---
 fs/ntfs3/record.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
index 6aa3a9d44df1..973e2a371bc2 100644
--- a/fs/ntfs3/record.c
+++ b/fs/ntfs3/record.c
@@ -223,28 +223,19 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
 		prev_type = 0;
 		attr = Add2Ptr(rec, off);
 	} else {
-		/* Check if input attr inside record. */
+		/*
+		 * We don't need to check previous attr here. There is
+		 * a bounds checking in the previous round.
+		 */
 		off = PtrOffset(rec, attr);
-		if (off >= used)
-			return NULL;
 
 		asize = le32_to_cpu(attr->size);
-		if (asize < SIZEOF_RESIDENT) {
-			/* Impossible 'cause we should not return such attribute. */
-			return NULL;
-		}
-
-		/* Overflow check. */
-		if (off + asize < off)
-			return NULL;
 
 		prev_type = le32_to_cpu(attr->type);
 		attr = Add2Ptr(attr, asize);
 		off += asize;
 	}
 
-	asize = le32_to_cpu(attr->size);
-
 	/* Can we use the first field (attr->type). */
 	if (off + 8 > used) {
 		static_assert(ALIGN(sizeof(enum ATTR_TYPE), 8) == 8);
@@ -265,6 +256,12 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
 	if (t32 < prev_type)
 		return NULL;
 
+	asize = le32_to_cpu(attr->size);
+	if (asize < SIZEOF_RESIDENT) {
+		/* Impossible 'cause we should not return such attribute. */
+		return NULL;
+	}
+
 	/* Check overflow and boundary. */
 	if (off + asize < off || off + asize > used)
 		return NULL;
-- 
2.34.1

Re: [PATCH v2] ntfs3: Add bounds checking to mi_enum_attr()

[lei lu]

Hi all,

This patch ignores the last attr is just 8 bytes and type field
is ATTR_END. I have already send V2.

Re: [PATCH v2] ntfs3: Add bounds checking to mi_enum_attr()

[Konstantin Komarov]

On 23.08.2024 16:39, lei lu wrote:
> Added bounds checking to make sure that every attr don't stray beyond
> valid memory region.
>
> Signed-off-by: lei lu <llfamsec@gmail.com>
> ---
>   fs/ntfs3/record.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
> index 6aa3a9d44df1..973e2a371bc2 100644
> --- a/fs/ntfs3/record.c
> +++ b/fs/ntfs3/record.c
> @@ -223,28 +223,19 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
>   		prev_type = 0;
>   		attr = Add2Ptr(rec, off);
>   	} else {
> -		/* Check if input attr inside record. */
> +		/*
> +		 * We don't need to check previous attr here. There is
> +		 * a bounds checking in the previous round.
> +		 */
>   		off = PtrOffset(rec, attr);
> -		if (off >= used)
> -			return NULL;
>   
>   		asize = le32_to_cpu(attr->size);
> -		if (asize < SIZEOF_RESIDENT) {
> -			/* Impossible 'cause we should not return such attribute. */
> -			return NULL;
> -		}
> -
> -		/* Overflow check. */
> -		if (off + asize < off)
> -			return NULL;
>   
>   		prev_type = le32_to_cpu(attr->type);
>   		attr = Add2Ptr(attr, asize);
>   		off += asize;
>   	}
>   
> -	asize = le32_to_cpu(attr->size);
> -
>   	/* Can we use the first field (attr->type). */
>   	if (off + 8 > used) {
>   		static_assert(ALIGN(sizeof(enum ATTR_TYPE), 8) == 8);
> @@ -265,6 +256,12 @@ struct ATTRIB *mi_enum_attr(struct mft_inode *mi, struct ATTRIB *attr)
>   	if (t32 < prev_type)
>   		return NULL;
>   
> +	asize = le32_to_cpu(attr->size);
> +	if (asize < SIZEOF_RESIDENT) {
> +		/* Impossible 'cause we should not return such attribute. */
> +		return NULL;
> +	}
> +
>   	/* Check overflow and boundary. */
>   	if (off + asize < off || off + asize > used)
>   		return NULL;
Hi Lei Lu,
I saw your patch and have started working on it, including checks and 
testing.

Regards, Konstantin