[PATCH] btrfs-progs: mkfs: fix xattr enumeration

[PATCH] btrfs-progs: mkfs: fix xattr enumeration

[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

Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration

[Nikolay Borisov]

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;
> 

Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration

[Vladimir Panteleev]

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;
> >

Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration

[Nikolay Borisov]

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>

Re: [PATCH] btrfs-progs: mkfs: fix xattr enumeration

[David Sterba]

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.