All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix to avoid memory leakage in f2fs_listxattr
@ 2019-10-09  3:20 ` Randall Huang via Linux-f2fs-devel
  0 siblings, 0 replies; 8+ messages in thread
From: Randall Huang @ 2019-10-09  3:20 UTC (permalink / raw)
  To: jaegeuk, yuchao0, linux-f2fs-devel, linux-kernel; +Cc: huangrandall

In f2fs_listxattr, there is no boundary check before
memcpy e_name to buffer.
If the e_name_len is corrupted,
unexpected memory contents may be returned to the buffer.

Signed-off-by: Randall Huang <huangrandall@google.com>
---
 fs/f2fs/xattr.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index b32c45621679..acc3663970cd 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -538,8 +538,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 {
 	struct inode *inode = d_inode(dentry);
+	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
 	struct f2fs_xattr_entry *entry;
-	void *base_addr;
+	void *base_addr, *last_base_addr;
 	int error = 0;
 	size_t rest = buffer_size;
 
@@ -549,6 +550,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	if (error)
 		return error;
 
+	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
+
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
 			f2fs_xattr_handler(entry->e_name_index);
@@ -559,6 +562,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 		if (!handler || (handler->list && !handler->list(dentry)))
 			continue;
 
+		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
+			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
+			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
+						inode->i_ino);
+			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
+			error = -EFSCORRUPTED;
+			goto cleanup;
+		}
+
 		prefix = xattr_prefix(handler);
 		prefix_len = strlen(prefix);
 		size = prefix_len + entry->e_name_len + 1;
-- 
2.23.0.581.g78d2f28ef7-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [f2fs-dev] [PATCH] f2fs: fix to avoid memory leakage in f2fs_listxattr
@ 2019-10-09  3:20 ` Randall Huang via Linux-f2fs-devel
  0 siblings, 0 replies; 8+ messages in thread
From: Randall Huang via Linux-f2fs-devel @ 2019-10-09  3:20 UTC (permalink / raw)
  To: jaegeuk, yuchao0, linux-f2fs-devel, linux-kernel

In f2fs_listxattr, there is no boundary check before
memcpy e_name to buffer.
If the e_name_len is corrupted,
unexpected memory contents may be returned to the buffer.

Signed-off-by: Randall Huang <huangrandall@google.com>
---
 fs/f2fs/xattr.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index b32c45621679..acc3663970cd 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -538,8 +538,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 {
 	struct inode *inode = d_inode(dentry);
+	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
 	struct f2fs_xattr_entry *entry;
-	void *base_addr;
+	void *base_addr, *last_base_addr;
 	int error = 0;
 	size_t rest = buffer_size;
 
@@ -549,6 +550,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	if (error)
 		return error;
 
+	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
+
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
 			f2fs_xattr_handler(entry->e_name_index);
@@ -559,6 +562,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 		if (!handler || (handler->list && !handler->list(dentry)))
 			continue;
 
+		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
+			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
+			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
+						inode->i_ino);
+			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
+			error = -EFSCORRUPTED;
+			goto cleanup;
+		}
+
 		prefix = xattr_prefix(handler);
 		prefix_len = strlen(prefix);
 		size = prefix_len + entry->e_name_len + 1;
-- 
2.23.0.581.g78d2f28ef7-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: fix to avoid memory leakage in f2fs_listxattr
  2019-10-09  3:20 ` [f2fs-dev] " Randall Huang via Linux-f2fs-devel
@ 2019-10-15  7:22   ` Chao Yu
  -1 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-10-15  7:22 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

Hi Randall,

On 2019/10/9 11:20, Randall Huang wrote:
> In f2fs_listxattr, there is no boundary check before
> memcpy e_name to buffer.
> If the e_name_len is corrupted,
> unexpected memory contents may be returned to the buffer.
> 
> Signed-off-by: Randall Huang <huangrandall@google.com>
> ---
>  fs/f2fs/xattr.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index b32c45621679..acc3663970cd 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -538,8 +538,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>  ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  {
>  	struct inode *inode = d_inode(dentry);
> +	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>  	struct f2fs_xattr_entry *entry;
> -	void *base_addr;
> +	void *base_addr, *last_base_addr;
>  	int error = 0;
>  	size_t rest = buffer_size;
>  
> @@ -549,6 +550,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	if (error)
>  		return error;
>  
> +	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
> +
>  	list_for_each_xattr(entry, base_addr) {
>  		const struct xattr_handler *handler =
>  			f2fs_xattr_handler(entry->e_name_index);
> @@ -559,6 +562,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  		if (!handler || (handler->list && !handler->list(dentry)))
>  			continue;
>  
> +		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
> +			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
> +						inode->i_ino);
> +			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
> +			error = -EFSCORRUPTED;
> +			goto cleanup;
> +		}

Could you relocate sanity check to the place before we check handler? As I'm
thinking we should always check validation of current entry before using its
field (entry->index).

Thanks,

> +
>  		prefix = xattr_prefix(handler);
>  		prefix_len = strlen(prefix);
>  		size = prefix_len + entry->e_name_len + 1;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to avoid memory leakage in f2fs_listxattr
@ 2019-10-15  7:22   ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-10-15  7:22 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

Hi Randall,

On 2019/10/9 11:20, Randall Huang wrote:
> In f2fs_listxattr, there is no boundary check before
> memcpy e_name to buffer.
> If the e_name_len is corrupted,
> unexpected memory contents may be returned to the buffer.
> 
> Signed-off-by: Randall Huang <huangrandall@google.com>
> ---
>  fs/f2fs/xattr.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index b32c45621679..acc3663970cd 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -538,8 +538,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>  ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  {
>  	struct inode *inode = d_inode(dentry);
> +	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
>  	struct f2fs_xattr_entry *entry;
> -	void *base_addr;
> +	void *base_addr, *last_base_addr;
>  	int error = 0;
>  	size_t rest = buffer_size;
>  
> @@ -549,6 +550,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	if (error)
>  		return error;
>  
> +	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
> +
>  	list_for_each_xattr(entry, base_addr) {
>  		const struct xattr_handler *handler =
>  			f2fs_xattr_handler(entry->e_name_index);
> @@ -559,6 +562,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  		if (!handler || (handler->list && !handler->list(dentry)))
>  			continue;
>  
> +		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
> +			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
> +						inode->i_ino);
> +			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
> +			error = -EFSCORRUPTED;
> +			goto cleanup;
> +		}

Could you relocate sanity check to the place before we check handler? As I'm
thinking we should always check validation of current entry before using its
field (entry->index).

Thanks,

> +
>  		prefix = xattr_prefix(handler);
>  		prefix_len = strlen(prefix);
>  		size = prefix_len + entry->e_name_len + 1;
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] f2fs: fix to avoid memory leakage in f2fs_listxattr
  2019-10-15  7:22   ` [f2fs-dev] " Chao Yu
@ 2019-10-18  6:56     ` Randall Huang via Linux-f2fs-devel
  -1 siblings, 0 replies; 8+ messages in thread
From: Randall Huang @ 2019-10-18  6:56 UTC (permalink / raw)
  To: jaegeuk, yuchao0, linux-f2fs-devel, linux-kernel; +Cc: huangrandall

In f2fs_listxattr, there is no boundary check before
memcpy e_name to buffer.
If the e_name_len is corrupted,
unexpected memory contents may be returned to the buffer.

Signed-off-by: Randall Huang <huangrandall@google.com>
---
v2:
relocate sanity check
---
 fs/f2fs/xattr.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 181900af2576..296b3189448a 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -539,8 +539,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 {
 	struct inode *inode = d_inode(dentry);
+	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
 	struct f2fs_xattr_entry *entry;
-	void *base_addr;
+	void *base_addr, *last_base_addr;
 	int error = 0;
 	size_t rest = buffer_size;
 
@@ -550,6 +551,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	if (error)
 		return error;
 
+	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
+
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
 			f2fs_xattr_handler(entry->e_name_index);
@@ -557,6 +560,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 		size_t prefix_len;
 		size_t size;
 
+		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
+			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
+			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
+						inode->i_ino);
+			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
+			error = -EFSCORRUPTED;
+			goto cleanup;
+		}
+
 		if (!handler || (handler->list && !handler->list(dentry)))
 			continue;
 
-- 
2.23.0.866.gb869b98d4c-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [f2fs-dev] [PATCH v2] f2fs: fix to avoid memory leakage in f2fs_listxattr
@ 2019-10-18  6:56     ` Randall Huang via Linux-f2fs-devel
  0 siblings, 0 replies; 8+ messages in thread
From: Randall Huang via Linux-f2fs-devel @ 2019-10-18  6:56 UTC (permalink / raw)
  To: jaegeuk, yuchao0, linux-f2fs-devel, linux-kernel

In f2fs_listxattr, there is no boundary check before
memcpy e_name to buffer.
If the e_name_len is corrupted,
unexpected memory contents may be returned to the buffer.

Signed-off-by: Randall Huang <huangrandall@google.com>
---
v2:
relocate sanity check
---
 fs/f2fs/xattr.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 181900af2576..296b3189448a 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -539,8 +539,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 {
 	struct inode *inode = d_inode(dentry);
+	nid_t xnid = F2FS_I(inode)->i_xattr_nid;
 	struct f2fs_xattr_entry *entry;
-	void *base_addr;
+	void *base_addr, *last_base_addr;
 	int error = 0;
 	size_t rest = buffer_size;
 
@@ -550,6 +551,8 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	if (error)
 		return error;
 
+	last_base_addr = (void *)base_addr + XATTR_SIZE(xnid, inode);
+
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
 			f2fs_xattr_handler(entry->e_name_index);
@@ -557,6 +560,15 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 		size_t prefix_len;
 		size_t size;
 
+		if ((void *)(entry) + sizeof(__u32) > last_base_addr ||
+			(void *)XATTR_NEXT_ENTRY(entry) > last_base_addr) {
+			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has corrupted xattr",
+						inode->i_ino);
+			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
+			error = -EFSCORRUPTED;
+			goto cleanup;
+		}
+
 		if (!handler || (handler->list && !handler->list(dentry)))
 			continue;
 
-- 
2.23.0.866.gb869b98d4c-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] f2fs: fix to avoid memory leakage in f2fs_listxattr
  2019-10-18  6:56     ` [f2fs-dev] " Randall Huang via Linux-f2fs-devel
@ 2019-10-22  8:35       ` Chao Yu
  -1 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-10-22  8:35 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

On 2019/10/18 14:56, Randall Huang wrote:
> In f2fs_listxattr, there is no boundary check before
> memcpy e_name to buffer.
> If the e_name_len is corrupted,
> unexpected memory contents may be returned to the buffer.
> 
> Signed-off-by: Randall Huang <huangrandall@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: fix to avoid memory leakage in f2fs_listxattr
@ 2019-10-22  8:35       ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2019-10-22  8:35 UTC (permalink / raw)
  To: Randall Huang, jaegeuk, linux-f2fs-devel, linux-kernel

On 2019/10/18 14:56, Randall Huang wrote:
> In f2fs_listxattr, there is no boundary check before
> memcpy e_name to buffer.
> If the e_name_len is corrupted,
> unexpected memory contents may be returned to the buffer.
> 
> Signed-off-by: Randall Huang <huangrandall@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-22  8:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09  3:20 [PATCH] f2fs: fix to avoid memory leakage in f2fs_listxattr Randall Huang
2019-10-09  3:20 ` [f2fs-dev] " Randall Huang via Linux-f2fs-devel
2019-10-15  7:22 ` Chao Yu
2019-10-15  7:22   ` [f2fs-dev] " Chao Yu
2019-10-18  6:56   ` [PATCH v2] " Randall Huang
2019-10-18  6:56     ` [f2fs-dev] " Randall Huang via Linux-f2fs-devel
2019-10-22  8:35     ` Chao Yu
2019-10-22  8:35       ` [f2fs-dev] " Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.