NFSv4 setattr null-terminates strings

NFSv4 setattr null-terminates strings

[Dave Chiluk]

As of commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172, the SETATTR nfsv4
command now null terminates fattr_owner and fattr_owner_group.

Here is an example excerpt from a tcpdump
Opcode: SETATTR (34)
    stateid
        [StateID Hash: 0xafa9]
        seqid: 0x00000000
        Data: 000000000000000000000000
    obj_attributes
        attrmask
            recc_attr: FATTR4_OWNER_GROUP (37)
                fattr4_owner_group: groupname@domainname.com
                    length: 25

This means that even though there are actually 24 characters in
groupname@domainname.com, we now send 24 characters + 1 null character.
Hence the total length of 25.  Previously the client would send just the
24 characters and set length to 24.

This isn't an issue for communication with other Linux machines, but it
is an issue for interaction with AIX machines.  As is discussed here.
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1101292

I attempted to read the RFC available here
http://tools.ietf.org/html/rfc5661, but have not been able to find a
statement instructing one way or the other.

So the question becomes, is it correct for linux to be sending this null
terminator when read against the RFC, or is it AIX's responsibility to
handle null-terminated strings?

Thank you,
Dave.

Re: NFSv4 setattr null-terminates strings

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

On Fri, 2013-03-08 at 11:11 -0600, Dave Chiluk wrote:
> As of commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172, the SETATTR nfsv4
> command now null terminates fattr_owner and fattr_owner_group.
> 
> Here is an example excerpt from a tcpdump
> Opcode: SETATTR (34)
>     stateid
>         [StateID Hash: 0xafa9]
>         seqid: 0x00000000
>         Data: 000000000000000000000000
>     obj_attributes
>         attrmask
>             recc_attr: FATTR4_OWNER_GROUP (37)
>                 fattr4_owner_group: groupname@domainname.com
>                     length: 25
> 
> This means that even though there are actually 24 characters in
> groupname@domainname.com, we now send 24 characters + 1 null character.
> Hence the total length of 25.  Previously the client would send just the
> 24 characters and set length to 24.
> 
> This isn't an issue for communication with other Linux machines, but it
> is an issue for interaction with AIX machines.  As is discussed here.
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1101292
> 
> I attempted to read the RFC available here
> http://tools.ietf.org/html/rfc5661, but have not been able to find a
> statement instructing one way or the other.
> 
> So the question becomes, is it correct for linux to be sending this null
> terminator when read against the RFC, or is it AIX's responsibility to
> handle null-terminated strings?

The client is definitely in error here according to RFC3530. Please
check if the attached patch fixes the issue for you.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-the-string-length-returned-by-the-idmapper.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-the-string-length-returned-by-the-idmapper.patch", Size: 2303 bytes --]

From 90eabc5dfde58de5c37de3866f427da02e86ce17 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 8 Mar 2013 12:56:37 -0500
Subject: [PATCH] NFSv4: Fix the string length returned by the idmapper

Functions like nfs_map_uid_to_name() and nfs_map_gid_to_group() are
expected to return a string without any terminating NUL character.
Regression introduced by commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172
(NFS: Store the legacy idmapper result in the keyring).

Reported-by: Dave Chiluk <dave.chiluk@canonical.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Bryan Schumaker <bjschuma@netapp.com>
Cc: stable@vger.kernel.org [>=3.4]
---
 fs/nfs/idmap.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index dc0f98d..c516da5 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -726,9 +726,9 @@ out1:
 	return ret;
 }
 
-static int nfs_idmap_instantiate(struct key *key, struct key *authkey, char *data)
+static int nfs_idmap_instantiate(struct key *key, struct key *authkey, char *data, size_t datalen)
 {
-	return key_instantiate_and_link(key, data, strlen(data) + 1,
+	return key_instantiate_and_link(key, data, datalen,
 					id_resolver_cache->thread_keyring,
 					authkey);
 }
@@ -738,6 +738,7 @@ static int nfs_idmap_read_and_verify_message(struct idmap_msg *im,
 		struct key *key, struct key *authkey)
 {
 	char id_str[NFS_UINT_MAXLEN];
+	size_t len;
 	int ret = -ENOKEY;
 
 	/* ret = -ENOKEY */
@@ -747,13 +748,15 @@ static int nfs_idmap_read_and_verify_message(struct idmap_msg *im,
 	case IDMAP_CONV_NAMETOID:
 		if (strcmp(upcall->im_name, im->im_name) != 0)
 			break;
-		sprintf(id_str, "%d", im->im_id);
-		ret = nfs_idmap_instantiate(key, authkey, id_str);
+		/* Note: here we store the NUL terminator too */
+		len = sprintf(id_str, "%d", im->im_id) + 1;
+		ret = nfs_idmap_instantiate(key, authkey, id_str, len);
 		break;
 	case IDMAP_CONV_IDTONAME:
 		if (upcall->im_id != im->im_id)
 			break;
-		ret = nfs_idmap_instantiate(key, authkey, im->im_name);
+		len = strlen(im->im_name);
+		ret = nfs_idmap_instantiate(key, authkey, im->im_name, len);
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.8.1.4

Re: NFSv4 setattr null-terminates strings

[Dave Chiluk]

The patch definitely fixes the null termination issue.  Hopefully that
is the only issue with AIX.

Thanks,
Dave.

On 03/08/2013 12:33 PM, Myklebust, Trond wrote:
> On Fri, 2013-03-08 at 11:11 -0600, Dave Chiluk wrote:
>> As of commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172, the SETATTR nfsv4
>> command now null terminates fattr_owner and fattr_owner_group.
>>
>> Here is an example excerpt from a tcpdump
>> Opcode: SETATTR (34)
>>     stateid
>>         [StateID Hash: 0xafa9]
>>         seqid: 0x00000000
>>         Data: 000000000000000000000000
>>     obj_attributes
>>         attrmask
>>             recc_attr: FATTR4_OWNER_GROUP (37)
>>                 fattr4_owner_group: groupname@domainname.com
>>                     length: 25
>>
>> This means that even though there are actually 24 characters in
>> groupname@domainname.com, we now send 24 characters + 1 null character.
>> Hence the total length of 25.  Previously the client would send just the
>> 24 characters and set length to 24.
>>
>> This isn't an issue for communication with other Linux machines, but it
>> is an issue for interaction with AIX machines.  As is discussed here.
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1101292
>>
>> I attempted to read the RFC available here
>> http://tools.ietf.org/html/rfc5661, but have not been able to find a
>> statement instructing one way or the other.
>>
>> So the question becomes, is it correct for linux to be sending this null
>> terminator when read against the RFC, or is it AIX's responsibility to
>> handle null-terminated strings?
> 
> The client is definitely in error here according to RFC3530. Please
> check if the attached patch fixes the issue for you.
> 
> Cheers
>   Trond
> 

Re: NFSv4 setattr null-terminates strings

[Dave Chiluk]

On 03/08/2013 12:33 PM, Myklebust, Trond wrote:
> On Fri, 2013-03-08 at 11:11 -0600, Dave Chiluk wrote:
>> As of commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172, the SETATTR nfsv4
>> command now null terminates fattr_owner and fattr_owner_group.
>>
>> Here is an example excerpt from a tcpdump
>> Opcode: SETATTR (34)
>>     stateid
>>         [StateID Hash: 0xafa9]
>>         seqid: 0x00000000
>>         Data: 000000000000000000000000
>>     obj_attributes
>>         attrmask
>>             recc_attr: FATTR4_OWNER_GROUP (37)
>>                 fattr4_owner_group: groupname@domainname.com
>>                     length: 25
>>
>> This means that even though there are actually 24 characters in
>> groupname@domainname.com, we now send 24 characters + 1 null character.
>> Hence the total length of 25.  Previously the client would send just the
>> 24 characters and set length to 24.
>>
>> This isn't an issue for communication with other Linux machines, but it
>> is an issue for interaction with AIX machines.  As is discussed here.
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1101292
>>
>> I attempted to read the RFC available here
>> http://tools.ietf.org/html/rfc5661, but have not been able to find a
>> statement instructing one way or the other.
>>
>> So the question becomes, is it correct for linux to be sending this null
>> terminator when read against the RFC, or is it AIX's responsibility to
>> handle null-terminated strings?
> 
> The client is definitely in error here according to RFC3530. Please
> check if the attached patch fixes the issue for you.
> 
> Cheers
>   Trond
> 

Trond,

This appears to work great.  Can you give me an idea when this will get
pulled into the the linux-nfs tree and eventually the mainline tree?

Dave.

Re: NFSv4 setattr null-terminates strings

[Dave Chiluk]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On 03/08/2013 12:33 PM, Myklebust, Trond wrote:
> On Fri, 2013-03-08 at 11:11 -0600, Dave Chiluk wrote:
>> As of commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172, the SETATTR nfsv4
>> command now null terminates fattr_owner and fattr_owner_group.
>>
>> Here is an example excerpt from a tcpdump
>> Opcode: SETATTR (34)
>>     stateid
>>         [StateID Hash: 0xafa9]
>>         seqid: 0x00000000
>>         Data: 000000000000000000000000
>>     obj_attributes
>>         attrmask
>>             recc_attr: FATTR4_OWNER_GROUP (37)
>>                 fattr4_owner_group: groupname@domainname.com
>>                     length: 25
>>
>> This means that even though there are actually 24 characters in
>> groupname@domainname.com, we now send 24 characters + 1 null character.
>> Hence the total length of 25.  Previously the client would send just the
>> 24 characters and set length to 24.
>>
>> This isn't an issue for communication with other Linux machines, but it
>> is an issue for interaction with AIX machines.  As is discussed here.
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1101292
>>
>> I attempted to read the RFC available here
>> http://tools.ietf.org/html/rfc5661, but have not been able to find a
>> statement instructing one way or the other.
>>
>> So the question becomes, is it correct for linux to be sending this null
>> terminator when read against the RFC, or is it AIX's responsibility to
>> handle null-terminated strings?
> 
> The client is definitely in error here according to RFC3530. Please
> check if the attached patch fixes the issue for you.
> 
> Cheers
>   Trond
> 

Thanks again for the patch.  When will this get pushed upstream?  I'm
blocked by the upstream commit, since I don't want to pull this in as a
SAUCE patch.

Dave.

[-- Attachment #2: 0001-NFSv4-Fix-the-string-length-returned-by-the-idmapper.patch --]
[-- Type: text/x-patch, Size: 2240 bytes --]

>From 90eabc5dfde58de5c37de3866f427da02e86ce17 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 8 Mar 2013 12:56:37 -0500
Subject: [PATCH] NFSv4: Fix the string length returned by the idmapper

Functions like nfs_map_uid_to_name() and nfs_map_gid_to_group() are
expected to return a string without any terminating NUL character.
Regression introduced by commit 57e62324e469e092ecc6c94a7a86fe4bd6ac5172
(NFS: Store the legacy idmapper result in the keyring).

Reported-by: Dave Chiluk <dave.chiluk@canonical.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Bryan Schumaker <bjschuma@netapp.com>
Cc: stable@vger.kernel.org [>=3.4]
---
 fs/nfs/idmap.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index dc0f98d..c516da5 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -726,9 +726,9 @@ out1:
 	return ret;
 }
 
-static int nfs_idmap_instantiate(struct key *key, struct key *authkey, char *data)
+static int nfs_idmap_instantiate(struct key *key, struct key *authkey, char *data, size_t datalen)
 {
-	return key_instantiate_and_link(key, data, strlen(data) + 1,
+	return key_instantiate_and_link(key, data, datalen,
 					id_resolver_cache->thread_keyring,
 					authkey);
 }
@@ -738,6 +738,7 @@ static int nfs_idmap_read_and_verify_message(struct idmap_msg *im,
 		struct key *key, struct key *authkey)
 {
 	char id_str[NFS_UINT_MAXLEN];
+	size_t len;
 	int ret = -ENOKEY;
 
 	/* ret = -ENOKEY */
@@ -747,13 +748,15 @@ static int nfs_idmap_read_and_verify_message(struct idmap_msg *im,
 	case IDMAP_CONV_NAMETOID:
 		if (strcmp(upcall->im_name, im->im_name) != 0)
 			break;
-		sprintf(id_str, "%d", im->im_id);
-		ret = nfs_idmap_instantiate(key, authkey, id_str);
+		/* Note: here we store the NUL terminator too */
+		len = sprintf(id_str, "%d", im->im_id) + 1;
+		ret = nfs_idmap_instantiate(key, authkey, id_str, len);
 		break;
 	case IDMAP_CONV_IDTONAME:
 		if (upcall->im_id != im->im_id)
 			break;
-		ret = nfs_idmap_instantiate(key, authkey, im->im_name);
+		len = strlen(im->im_name);
+		ret = nfs_idmap_instantiate(key, authkey, im->im_name, len);
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.8.1.4