Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] btrfs-progs: mkfs: fix xattr enumeration
@ 2019-09-06  9:58 Vladimir Panteleev
  2019-09-09 11:22 ` Nikolay Borisov
  2019-09-09 17:35 ` David Sterba
  0 siblings, 2 replies; 5+ messages in thread
From: Vladimir Panteleev @ 2019-09-06  9:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Vladimir Panteleev

Use the return value of listxattr instead of tokenizing.

The end of the extended attribute list is indicated by the return
value, not an empty list item (two consecutive NULs). Using strtok
in this way thus sometimes caused add_xattr_item to reuse stack data
in xattr_list from the previous invocation, thus querying attributes
that are not actually in the file's xattr list.

Issue: #194
Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>
---
 mkfs/rootdir.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index 51411e02..c86159e7 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
 	int ret;
 	int cur_name_len;
 	char xattr_list[XATTR_LIST_MAX];
+	char *xattr_list_end;
 	char *cur_name;
 	char cur_value[XATTR_SIZE_MAX];
-	char delimiter = '\0';
-	char *next_location = xattr_list;
 
 	ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX);
 	if (ret < 0) {
@@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
 	if (ret == 0)
 		return ret;
 
-	cur_name = strtok(xattr_list, &delimiter);
-	while (cur_name != NULL) {
+	xattr_list_end = xattr_list + ret;
+	cur_name = xattr_list;
+	while (cur_name < xattr_list_end) {
 		cur_name_len = strlen(cur_name);
-		next_location += cur_name_len + 1;
 
 		ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX);
 		if (ret < 0) {
@@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
 					file_name);
 		}
 
-		cur_name = strtok(next_location, &delimiter);
+		cur_name += cur_name_len + 1;
 	}
 
 	return ret;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration
  2019-09-06  9:58 [PATCH] btrfs-progs: mkfs: fix xattr enumeration Vladimir Panteleev
@ 2019-09-09 11:22 ` Nikolay Borisov
  2019-09-09 11:32   ` Vladimir Panteleev
  2019-09-09 17:35 ` David Sterba
  1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2019-09-09 11:22 UTC (permalink / raw)
  To: Vladimir Panteleev, linux-btrfs



On 6.09.19 г. 12:58 ч., Vladimir Panteleev wrote:
> Use the return value of listxattr instead of tokenizing.
> 
> The end of the extended attribute list is indicated by the return
> value, not an empty list item (two consecutive NULs). Using strtok
> in this way thus sometimes caused add_xattr_item to reuse stack data
> in xattr_list from the previous invocation, thus querying attributes
> that are not actually in the file's xattr list.
> 
> Issue: #194
> Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>

Can you elaborate how to trigger this? I tried by creating a folder with
2 files and set 5 xattr to the first file and 1 to the second. Then I
run mkfs.btrfs -r /path/to/dir /dev/vdc and stepped through the code
with gdb and didn't see any issues. Ideally I'd like to see a regression
test for this issue.

Your code looks correct.

> ---
>  mkfs/rootdir.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
> index 51411e02..c86159e7 100644
> --- a/mkfs/rootdir.c
> +++ b/mkfs/rootdir.c
> @@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
>  	int ret;
>  	int cur_name_len;
>  	char xattr_list[XATTR_LIST_MAX];
> +	char *xattr_list_end;
>  	char *cur_name;
>  	char cur_value[XATTR_SIZE_MAX];
> -	char delimiter = '\0';
> -	char *next_location = xattr_list;
>  
>  	ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX);
>  	if (ret < 0) {
> @@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
>  	if (ret == 0)
>  		return ret;
>  
> -	cur_name = strtok(xattr_list, &delimiter);
> -	while (cur_name != NULL) {
> +	xattr_list_end = xattr_list + ret;
> +	cur_name = xattr_list;
> +	while (cur_name < xattr_list_end) {
>  		cur_name_len = strlen(cur_name);
> -		next_location += cur_name_len + 1;
>  
>  		ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX);
>  		if (ret < 0) {
> @@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
>  					file_name);
>  		}
>  
> -		cur_name = strtok(next_location, &delimiter);
> +		cur_name += cur_name_len + 1;
>  	}
>  
>  	return ret;
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration
  2019-09-09 11:22 ` Nikolay Borisov
@ 2019-09-09 11:32   ` Vladimir Panteleev
  2019-09-09 12:16     ` Nikolay Borisov
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Panteleev @ 2019-09-09 11:32 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: Btrfs BTRFS

Hi Nikolay,

Unfortunately, as I mentioned in the issue, I have also been unable to
reproduce this issue locally.

Please see here:
https://github.com/kdave/btrfs-progs/issues/194

The reporter tested the patch and confirmed that it worked.
Additionally, they have provided strace output which appears to
confirm that the bug description in the patch commit message indeed
corresponds to the behavior they are observing on their machine.

Perhaps the issue might be reproducible in an environment closer to
the reporter's (looks like some Fedora distro judging by the uname).

On Mon, 9 Sep 2019 at 11:22, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
>
> On 6.09.19 г. 12:58 ч., Vladimir Panteleev wrote:
> > Use the return value of listxattr instead of tokenizing.
> >
> > The end of the extended attribute list is indicated by the return
> > value, not an empty list item (two consecutive NULs). Using strtok
> > in this way thus sometimes caused add_xattr_item to reuse stack data
> > in xattr_list from the previous invocation, thus querying attributes
> > that are not actually in the file's xattr list.
> >
> > Issue: #194
> > Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>
>
> Can you elaborate how to trigger this? I tried by creating a folder with
> 2 files and set 5 xattr to the first file and 1 to the second. Then I
> run mkfs.btrfs -r /path/to/dir /dev/vdc and stepped through the code
> with gdb and didn't see any issues. Ideally I'd like to see a regression
> test for this issue.
>
> Your code looks correct.
>
> > ---
> >  mkfs/rootdir.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
> > index 51411e02..c86159e7 100644
> > --- a/mkfs/rootdir.c
> > +++ b/mkfs/rootdir.c
> > @@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
> >       int ret;
> >       int cur_name_len;
> >       char xattr_list[XATTR_LIST_MAX];
> > +     char *xattr_list_end;
> >       char *cur_name;
> >       char cur_value[XATTR_SIZE_MAX];
> > -     char delimiter = '\0';
> > -     char *next_location = xattr_list;
> >
> >       ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX);
> >       if (ret < 0) {
> > @@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
> >       if (ret == 0)
> >               return ret;
> >
> > -     cur_name = strtok(xattr_list, &delimiter);
> > -     while (cur_name != NULL) {
> > +     xattr_list_end = xattr_list + ret;
> > +     cur_name = xattr_list;
> > +     while (cur_name < xattr_list_end) {
> >               cur_name_len = strlen(cur_name);
> > -             next_location += cur_name_len + 1;
> >
> >               ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX);
> >               if (ret < 0) {
> > @@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans,
> >                                       file_name);
> >               }
> >
> > -             cur_name = strtok(next_location, &delimiter);
> > +             cur_name += cur_name_len + 1;
> >       }
> >
> >       return ret;
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration
  2019-09-09 11:32   ` Vladimir Panteleev
@ 2019-09-09 12:16     ` Nikolay Borisov
  0 siblings, 0 replies; 5+ messages in thread
From: Nikolay Borisov @ 2019-09-09 12:16 UTC (permalink / raw)
  To: Vladimir Panteleev; +Cc: Btrfs BTRFS



On 9.09.19 г. 14:32 ч., Vladimir Panteleev wrote:
> Hi Nikolay,
> 
> Unfortunately, as I mentioned in the issue, I have also been unable to
> reproduce this issue locally.
> 
> Please see here:
> https://github.com/kdave/btrfs-progs/issues/194
> 
> The reporter tested the patch and confirmed that it worked.
> Additionally, they have provided strace output which appears to
> confirm that the bug description in the patch commit message indeed
> corresponds to the behavior they are observing on their machine.
> 
> Perhaps the issue might be reproducible in an environment closer to
> the reporter's (looks like some Fedora distro judging by the uname).



Right, looking at the kernel portion listxattr just copies the attribute
names into the passed user pointer. So if the data copied is less than
XATTR_LIST_MAX then the data beyond the return value of llistxattr is
undefined which could cause the described problem.


I agree that your code is more correct as it handles data only in the
range [0...ret].


Consider this patch:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration
  2019-09-06  9:58 [PATCH] btrfs-progs: mkfs: fix xattr enumeration Vladimir Panteleev
  2019-09-09 11:22 ` Nikolay Borisov
@ 2019-09-09 17:35 ` David Sterba
  1 sibling, 0 replies; 5+ messages in thread
From: David Sterba @ 2019-09-09 17:35 UTC (permalink / raw)
  To: Vladimir Panteleev; +Cc: linux-btrfs

On Fri, Sep 06, 2019 at 09:58:46AM +0000, Vladimir Panteleev wrote:
> Use the return value of listxattr instead of tokenizing.
> 
> The end of the extended attribute list is indicated by the return
> value, not an empty list item (two consecutive NULs). Using strtok
> in this way thus sometimes caused add_xattr_item to reuse stack data
> in xattr_list from the previous invocation, thus querying attributes
> that are not actually in the file's xattr list.
> 
> Issue: #194
> Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md>

Added to devel, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06  9:58 [PATCH] btrfs-progs: mkfs: fix xattr enumeration Vladimir Panteleev
2019-09-09 11:22 ` Nikolay Borisov
2019-09-09 11:32   ` Vladimir Panteleev
2019-09-09 12:16     ` Nikolay Borisov
2019-09-09 17:35 ` David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox