* [PATCH v1] nfs: nfs client decode fslocations oops if server cheating it
2013-03-25 10:57 [PATCH] nfs: nfs client decode fslocations oops if server cheating it fanchaoting
@ 2013-03-27 4:21 ` fanchaoting
2013-03-27 16:17 ` [PATCH] " Myklebust, Trond
1 sibling, 0 replies; 3+ messages in thread
From: fanchaoting @ 2013-03-27 4:21 UTC (permalink / raw)
To: Myklebust, Trond, bfields@fieldses.org; +Cc: linux-nfs@vger.kernel.org
now nfs server will return wrong nlocations,nservers, ncomponents to
the client.for example if the nlocations is NFS4_FS_LOCATIONS_MAXENTRIES,
the nfs client will decode oops when run "struct nfs4_fs_location *loc
= &res->locations[res->nlocations]"
#################################################################
3599 if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
3600 res->nlocations++;
#################################################################
i see if res->nlocations is NFS4_FS_LOCATIONS_MAXENTRIES -1, then next it will
run res->nlocations++ and res->nlocations will be NFS4_FS_LOCATIONS_MAXENTRIES.
if res->nlocations is NFS4_FS_LOCATIONS_MAXENTRIES , it maybe oops when run following
code.
#################################################################
...snip...
3562 u32 m;
3563 ★ struct nfs4_fs_location *loc = &res->locations[res->nlocations]; ★<--bug ,max location is NFS4_FS_LOCATIONS_MAXENTRIES-1,but now res->nlocations is NFS4_FS_LOCATIONS_MAXENTRIES
35
3565 p = xdr_inline_decode(xdr, 4);
3566 if (unlikely(!p))
3567 goto out_overflow;
3568 m = be32_to_cpup(p);
3569
3570 ★ loc->nservers = 0;<--it maybe cause oops.
...snip...
#################################################
Signed-off-by: fanchaoting <fanchaoting@cn.fujitsu.com>
Reviewed-by: chendt.fnst <chendt.fnst@cn.fujitsu.com>
---
fs/nfs/nfs4xdr.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index e3edda5..25f1769 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3496,6 +3496,10 @@ static int decode_pathname(struct xdr_stream *xdr, struct nfs4_pathname *path)
n = be32_to_cpup(p);
if (n == 0)
goto root_path;
+ if (n > NFS4_PATHNAME_MAXCOMPONENTS) {
+ dprintk("%s: server cheating client ncomponents :%d\n", __func__, n);
+ goto out_eio;
+ }
dprintk("pathname4: ");
path->ncomponents = 0;
while (path->ncomponents < n) {
@@ -3557,6 +3561,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
n = be32_to_cpup(p);
if (n <= 0)
goto out_eio;
+ if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
+ dprintk("%s: server cheating client nlocations :%d\n", __func__, n);
+ goto out_eio;
+ }
res->nlocations = 0;
while (res->nlocations < n) {
u32 m;
@@ -3566,7 +3574,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(!p))
goto out_overflow;
m = be32_to_cpup(p);
-
+ if (m > NFS4_FS_LOCATION_MAXSERVERS) {
+ dprintk("%s: server cheating client nservers :%d\n", __func__, m);
+ goto out_eio;
+ }
loc->nservers = 0;
dprintk("%s: servers:\n", __func__);
while (loc->nservers < m) {
-- 1.7.1 --
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] nfs: nfs client decode fslocations oops if server cheating it
2013-03-25 10:57 [PATCH] nfs: nfs client decode fslocations oops if server cheating it fanchaoting
2013-03-27 4:21 ` [PATCH v1] " fanchaoting
@ 2013-03-27 16:17 ` Myklebust, Trond
1 sibling, 0 replies; 3+ messages in thread
From: Myklebust, Trond @ 2013-03-27 16:17 UTC (permalink / raw)
To: fanchaoting; +Cc: bfields@fieldses.org, linux-nfs@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 761 bytes --]
On Mon, 2013-03-25 at 18:57 +0800, fanchaoting wrote:
> now nfs server will return wrong nlocations,nservers, ncomponents to the client.
These limits are imposed by the client. The server knows nothing about
them as they are not part of the NFSv4 spec.
> for example if the nlocations is NFS4_FS_LOCATIONS_MAXENTRIES, the nfs client
> will decode oops when run "struct nfs4_fs_location *loc = &res->locations[res->nlocations]"
Your patch means that instead of making a best effort attempt to decode
what the server sends us, we end up decoding nothing at all and just
reporting an error.
How about something like the following instead?
--
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-Oopses-in-the-fs_locations-code.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-Oopses-in-the-fs_locations-code.patch", Size: 3995 bytes --]
From 9bbcf2595ca1d90675b30ac4c08de8be3636ad48 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 27 Mar 2013 11:54:45 -0400
Subject: [PATCH] NFSv4: Fix Oopses in the fs_locations code
If the server sends us a pathname with more components than the client
limit of NFS4_PATHNAME_MAXCOMPONENTS, more server entries than the client
limit of NFS4_FS_LOCATION_MAXSERVERS, or sends a total number of
fs_locations entries than the client limit of NFS4_FS_LOCATIONS_MAXENTRIES
then we will currently Oops because the limit checks are done _after_ we've
decoded the data into the arrays.
Reported-by: fanchaoting<fanchaoting@cn.fujitsu.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
fs/nfs/nfs4xdr.c | 43 ++++++++++++++++++++-----------------------
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 235ed59..7da8324 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3496,8 +3496,11 @@ static int decode_pathname(struct xdr_stream *xdr, struct nfs4_pathname *path)
if (n == 0)
goto root_path;
dprintk("pathname4: ");
- path->ncomponents = 0;
- while (path->ncomponents < n) {
+ if (n > NFS4_PATHNAME_MAXCOMPONENTS) {
+ dprintk("cannot parse %d components in path\n", n);
+ goto out_eio;
+ }
+ for (path->ncomponents = 0; path->ncomponents < n; path->ncomponents++) {
struct nfs4_string *component = &path->components[path->ncomponents];
status = decode_opaque_inline(xdr, &component->len, &component->data);
if (unlikely(status != 0))
@@ -3506,12 +3509,6 @@ static int decode_pathname(struct xdr_stream *xdr, struct nfs4_pathname *path)
pr_cont("%s%.*s ",
(path->ncomponents != n ? "/ " : ""),
component->len, component->data);
- if (path->ncomponents < NFS4_PATHNAME_MAXCOMPONENTS)
- path->ncomponents++;
- else {
- dprintk("cannot parse %d components in path\n", n);
- goto out_eio;
- }
}
out:
return status;
@@ -3556,27 +3553,23 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
n = be32_to_cpup(p);
if (n <= 0)
goto out_eio;
- res->nlocations = 0;
- while (res->nlocations < n) {
+ for (res->nlocations = 0; res->nlocations < n; res->nlocations++) {
u32 m;
- struct nfs4_fs_location *loc = &res->locations[res->nlocations];
+ struct nfs4_fs_location *loc;
+ if (res->nlocations == NFS4_FS_LOCATIONS_MAXENTRIES)
+ break;
+ loc = &res->locations[res->nlocations];
p = xdr_inline_decode(xdr, 4);
if (unlikely(!p))
goto out_overflow;
m = be32_to_cpup(p);
- loc->nservers = 0;
dprintk("%s: servers:\n", __func__);
- while (loc->nservers < m) {
- struct nfs4_string *server = &loc->servers[loc->nservers];
- status = decode_opaque_inline(xdr, &server->len, &server->data);
- if (unlikely(status != 0))
- goto out_eio;
- dprintk("%s ", server->data);
- if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
- loc->nservers++;
- else {
+ for (loc->nservers = 0; loc->nservers < m; loc->nservers++) {
+ struct nfs4_string *server;
+
+ if (loc->nservers == NFS4_FS_LOCATION_MAXSERVERS) {
unsigned int i;
dprintk("%s: using first %u of %u servers "
"returned for location %u\n",
@@ -3590,13 +3583,17 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
if (unlikely(status != 0))
goto out_eio;
}
+ break;
}
+ server = &loc->servers[loc->nservers];
+ status = decode_opaque_inline(xdr, &server->len, &server->data);
+ if (unlikely(status != 0))
+ goto out_eio;
+ dprintk("%s ", server->data);
}
status = decode_pathname(xdr, &loc->rootpath);
if (unlikely(status != 0))
goto out_eio;
- if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
- res->nlocations++;
}
if (res->nlocations != 0)
status = NFS_ATTR_FATTR_V4_LOCATIONS;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread