[PATCH 0/2] ocfs2: add checks in ocfs2_xattr_find_entry() to avoid potential out-of-bound access.

[PATCH 0/2] ocfs2: add checks in ocfs2_xattr_find_entry() to avoid potential out-of-bound access.

[Ferry Meng]

Hi, all:

This patch series attempts to address a scenario where accessing user-defined 
xattrs in a carefully crafted image can lead to out-of-bound access.(To speak 
truthfully, I do not think this vehavior would occur under proper usage.) 

In my testing environment, I constructed an OCFS2 image, created a file with 
several user-defined xattrs(long name attributes, this will cause a "Non-INLINE"
xattr, which requires additional space for storage), and then forcibly modified
the xe_name_offset using a binary editing tool (e.g "hexedit"). Upon remounting 
the image and running 'getfattr -d /path/to/file', this patchset was able to 
detect "partial" malicious modification.

Comments and feedbacks are welcomed.

Ferry Meng (2):
  ocfs2: add bounds checking to ocfs2_xattr_find_entry()
  ocfs2: strict bound check before memcmp in ocfs2_xattr_find_entry()

 fs/ocfs2/xattr.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH 1/2] ocfs2: add bounds checking to ocfs2_xattr_find_entry()

[Ferry Meng]

Just add redundant (perhaps paranoia) checks to make sure it doesn't
stray beyond valid meory region of ocfs2 xattr entry array during a
single match.

Maybe this patch can prevent some crash caused by crafted poison images.

Signed-off-by: Ferry Meng <mengferry@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 3b81213ed7b8..37be4a286faf 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1062,8 +1062,8 @@ ssize_t ocfs2_listxattr(struct dentry *dentry,
 	return i_ret + b_ret;
 }
 
-static int ocfs2_xattr_find_entry(int name_index,
-				  const char *name,
+static int ocfs2_xattr_find_entry(struct inode *inode, void *end,
+				  int name_index, const char *name,
 				  struct ocfs2_xattr_search *xs)
 {
 	struct ocfs2_xattr_entry *entry;
@@ -1076,6 +1076,10 @@ static int ocfs2_xattr_find_entry(int name_index,
 	name_len = strlen(name);
 	entry = xs->here;
 	for (i = 0; i < le16_to_cpu(xs->header->xh_count); i++) {
+		if ((void *)entry >= end) {
+			ocfs2_error(inode->i_sb, "corrupted xattr entries");
+			return -EFSCORRUPTED;
+		}
 		cmp = name_index - ocfs2_xattr_get_type(entry);
 		if (!cmp)
 			cmp = name_len - entry->xe_name_len;
@@ -1166,7 +1170,7 @@ static int ocfs2_xattr_ibody_get(struct inode *inode,
 	xs->base = (void *)xs->header;
 	xs->here = xs->header->xh_entries;
 
-	ret = ocfs2_xattr_find_entry(name_index, name, xs);
+	ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
 	if (ret)
 		return ret;
 	size = le64_to_cpu(xs->here->xe_value_size);
@@ -2698,7 +2702,7 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
 
 	/* Find the named attribute. */
 	if (oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) {
-		ret = ocfs2_xattr_find_entry(name_index, name, xs);
+		ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
 		if (ret && ret != -ENODATA)
 			return ret;
 		xs->not_found = ret;
@@ -2833,7 +2837,7 @@ static int ocfs2_xattr_block_find(struct inode *inode,
 		xs->end = (void *)(blk_bh->b_data) + blk_bh->b_size;
 		xs->here = xs->header->xh_entries;
 
-		ret = ocfs2_xattr_find_entry(name_index, name, xs);
+		ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
 	} else
 		ret = ocfs2_xattr_index_block_find(inode, blk_bh,
 						   name_index,
-- 
2.32.0.3.g01195cf9f

[PATCH 2/2] ocfs2: strict bound check before memcmp in ocfs2_xattr_find_entry()

[Ferry Meng]

xattr in ocfs2 maybe not INLINE, but saved with additional space
requested. It's better to check if the memory is out of bound before
memcmp, although this possibility mainly comes from custom poisonous
images.

Signed-off-by: Ferry Meng <mengferry@linux.alibaba.com>
---
 fs/ocfs2/xattr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 37be4a286faf..4ceb0cb4cb71 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1083,10 +1083,15 @@ static int ocfs2_xattr_find_entry(struct inode *inode, void *end,
 		cmp = name_index - ocfs2_xattr_get_type(entry);
 		if (!cmp)
 			cmp = name_len - entry->xe_name_len;
-		if (!cmp)
+		if (!cmp) {
+			if ((xs->base + le16_to_cpu(entry->xe_name_offset) + name_len) > end) {
+				ocfs2_error(inode->i_sb, "corrupted xattr entries");
+				return -EFSCORRUPTED;
+			}
 			cmp = memcmp(name, (xs->base +
 				     le16_to_cpu(entry->xe_name_offset)),
 				     name_len);
+		}
 		if (cmp == 0)
 			break;
 		entry += 1;
-- 
2.32.0.3.g01195cf9f

Re: [PATCH 1/2] ocfs2: add bounds checking to ocfs2_xattr_find_entry()

[Joseph Qi]

On 5/15/24 9:29 PM, Ferry Meng wrote:
> Just add redundant (perhaps paranoia) checks to make sure it doesn't
> stray beyond valid meory region of ocfs2 xattr entry array during a
> single match.
> 
> Maybe this patch can prevent some crash caused by crafted poison images.
> 

I'd rather restructure the commit message as below:

Add a paranoia check to make sure it doesn't stray beyond valid memory
region containing ocfs2 xattr entries when scanning for a match.
It will prevent out-of-bound access in case of crafted images.

> Signed-off-by: Ferry Meng <mengferry@linux.alibaba.com>
> ---
>  fs/ocfs2/xattr.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 3b81213ed7b8..37be4a286faf 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -1062,8 +1062,8 @@ ssize_t ocfs2_listxattr(struct dentry *dentry,
>  	return i_ret + b_ret;
>  }
>  
> -static int ocfs2_xattr_find_entry(int name_index,
> -				  const char *name,
> +static int ocfs2_xattr_find_entry(struct inode *inode, void *end,

'end' can be obtained from ocfs2_xattr_search directly.

Thanks,
Joseph

> +				  int name_index, const char *name,
>  				  struct ocfs2_xattr_search *xs)
>  {
>  	struct ocfs2_xattr_entry *entry;
> @@ -1076,6 +1076,10 @@ static int ocfs2_xattr_find_entry(int name_index,
>  	name_len = strlen(name);
>  	entry = xs->here;
>  	for (i = 0; i < le16_to_cpu(xs->header->xh_count); i++) {
> +		if ((void *)entry >= end) {
> +			ocfs2_error(inode->i_sb, "corrupted xattr entries");
> +			return -EFSCORRUPTED;
> +		}
>  		cmp = name_index - ocfs2_xattr_get_type(entry);
>  		if (!cmp)
>  			cmp = name_len - entry->xe_name_len;
> @@ -1166,7 +1170,7 @@ static int ocfs2_xattr_ibody_get(struct inode *inode,
>  	xs->base = (void *)xs->header;
>  	xs->here = xs->header->xh_entries;
>  
> -	ret = ocfs2_xattr_find_entry(name_index, name, xs);
> +	ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
>  	if (ret)
>  		return ret;
>  	size = le64_to_cpu(xs->here->xe_value_size);
> @@ -2698,7 +2702,7 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
>  
>  	/* Find the named attribute. */
>  	if (oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) {
> -		ret = ocfs2_xattr_find_entry(name_index, name, xs);
> +		ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
>  		if (ret && ret != -ENODATA)
>  			return ret;
>  		xs->not_found = ret;
> @@ -2833,7 +2837,7 @@ static int ocfs2_xattr_block_find(struct inode *inode,
>  		xs->end = (void *)(blk_bh->b_data) + blk_bh->b_size;
>  		xs->here = xs->header->xh_entries;
>  
> -		ret = ocfs2_xattr_find_entry(name_index, name, xs);
> +		ret = ocfs2_xattr_find_entry(inode, xs->end, name_index, name, xs);
>  	} else
>  		ret = ocfs2_xattr_index_block_find(inode, blk_bh,
>  						   name_index,

Re: [PATCH 2/2] ocfs2: strict bound check before memcmp in ocfs2_xattr_find_entry()

[Joseph Qi]

On 5/15/24 9:29 PM, Ferry Meng wrote:
> xattr in ocfs2 maybe not INLINE, but saved with additional space
> requested. It's better to check if the memory is out of bound before
> memcmp, although this possibility mainly comes from custom poisonous
> images.

Specifically, this only addresses the case non-indexed xattr. 

> 
> Signed-off-by: Ferry Meng <mengferry@linux.alibaba.com>
> ---
>  fs/ocfs2/xattr.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 37be4a286faf..4ceb0cb4cb71 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -1083,10 +1083,15 @@ static int ocfs2_xattr_find_entry(struct inode *inode, void *end,
>  		cmp = name_index - ocfs2_xattr_get_type(entry);

Or define a local variable 'offset' for le16_to_cpu(entry->xe_name_offset).

Thanks,
Joseph

>  		if (!cmp)
>  			cmp = name_len - entry->xe_name_len;
> -		if (!cmp)
> +		if (!cmp) {
> +			if ((xs->base + le16_to_cpu(entry->xe_name_offset) + name_len) > end) {
> +				ocfs2_error(inode->i_sb, "corrupted xattr entries");
> +				return -EFSCORRUPTED;
> +			}
>  			cmp = memcmp(name, (xs->base +
>  				     le16_to_cpu(entry->xe_name_offset)),
>  				     name_len);
> +		}
>  		if (cmp == 0)
>  			break;
>  		entry += 1;