[PATCH] nfs: Fix getxattr kernel panic and memory overflow

[PATCH] nfs: Fix getxattr kernel panic and memory overflow

[Jeffrey Mitchell]

_nfs4_get_security_label() and decode_attr_security_label() run severe
risks with memory management and do not fully implement their
functionality. In the case that the buffer and length are both NULL, which
according to the getxattr man page should simply return the length of the
attribute, decode_attr_security_label() will kernel panic trying to write
to the null pointer. If the buffer length is nonzero but below the size of
the attribute, decode_attr_security_label() will write the data anyway,
overflowing the buffer, and it isn't until later in
_nfs4_get_security_label() that -ERANGE gets returned.

Here is some of the kernel panic output that I reproduced:
BUG: kernel NULL pointer dereference, address: 0000000000000000
[...]
RIP: 0010:__memcpy+0x12/0x20
[...]
Call Trace:
 decode_getfattr_attrs+0xb1f/0xdc0
 ? set_next_entity+0x8e/0x180
 decode_getfattr_generic.constprop.0+0x10f/0x210
 ? rpc_decode_header+0x570/0x570
 nfs4_xdr_dec_getattr+0x94/0xa0
[...]
 _nfs4_get_security_label+0x134/0x180
 ? _cond_resched+0x10/0x20
 ? __kmalloc+0x1f6/0x200
 nfs4_xattr_get_nfs4_label+0x89/0x120
 __vfs_getxattr+0x4e/0x70
 ecryptfs_getxattr_lower+0x4a/0x70
 ecryptfs_xattr_get+0x23/0x24
 __vfs_getxattr+0x4e/0x70
 sb_finish_set_opts+0x12c/0x240
 selinux_set_mnt_opts+0x288/0x6a0
 security_sb_set_mnt_opts+0x40/0x60
 vfs_get_tree+0x57/0xb0
 do_mount+0x742/0x9b0
 __x64_sys_mount+0x89/0xc0
 do_syscall_64+0x3e/0x70

- Jeffrey

Jeffrey Mitchell (1):
  nfs: Fix getxattr kernel panic and memory overflow

 fs/nfs/nfs4proc.c | 2 --
 fs/nfs/nfs4xdr.c  | 5 ++++-
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH] nfs: Fix getxattr kernel panic and memory overflow

[Jeffrey Mitchell]

Move the buffer size check to decode_attr_security_label() before memcpy()
Only call memcpy() if the buffer is large enough

Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
---
 fs/nfs/nfs4proc.c | 2 --
 fs/nfs/nfs4xdr.c  | 5 ++++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2e2dac29a9e9..45e0585e0667 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5845,8 +5845,6 @@ static int _nfs4_get_security_label(struct inode *inode, void *buf,
 		return ret;
 	if (!(fattr.valid & NFS_ATTR_FATTR_V4_SECURITY_LABEL))
 		return -ENOENT;
-	if (buflen < label.len)
-		return -ERANGE;
 	return 0;
 }
 
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 47817ef0aadb..50162e0a959c 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -4166,7 +4166,10 @@ static int decode_attr_security_label(struct xdr_stream *xdr, uint32_t *bitmap,
 			return -EIO;
 		if (len < NFS4_MAXLABELLEN) {
 			if (label) {
-				memcpy(label->label, p, len);
+				if (label->len && label->len < len)
+					return -ERANGE;
+				if (label->len)
+					memcpy(label->label, p, len);
 				label->len = len;
 				label->pi = pi;
 				label->lfs = lfs;
-- 
2.25.1

Re: [PATCH] nfs: Fix getxattr kernel panic and memory overflow

[Trond Myklebust]

On Wed, 2020-08-05 at 12:23 -0500, Jeffrey Mitchell wrote:
> Move the buffer size check to decode_attr_security_label() before
> memcpy()
> Only call memcpy() if the buffer is large enough
> 
> Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
> ---
>  fs/nfs/nfs4proc.c | 2 --
>  fs/nfs/nfs4xdr.c  | 5 ++++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 2e2dac29a9e9..45e0585e0667 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5845,8 +5845,6 @@ static int _nfs4_get_security_label(struct
> inode *inode, void *buf,
>  		return ret;
>  	if (!(fattr.valid & NFS_ATTR_FATTR_V4_SECURITY_LABEL))
>  		return -ENOENT;
> -	if (buflen < label.len)
> -		return -ERANGE;
>  	return 0;
>  }
>  
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 47817ef0aadb..50162e0a959c 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -4166,7 +4166,10 @@ static int decode_attr_security_label(struct
> xdr_stream *xdr, uint32_t *bitmap,
>  			return -EIO;
>  		if (len < NFS4_MAXLABELLEN) {
>  			if (label) {
> -				memcpy(label->label, p, len);
> +				if (label->len && label->len < len)
> +					return -ERANGE;
> +				if (label->len)
> +					memcpy(label->label, p, len);

Just a heads up that I fixed this to avoid the duplicate test of label-
>len != 0 and I added a Fixes: before applying.

>  				label->len = len;
>  				label->pi = pi;
>  				label->lfs = lfs;
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com