2013-03-25 10:57:58

by fanchaoting

[permalink] [raw]
Subject: [PATCH] nfs: nfs client decode fslocations oops if server cheating it

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]"

Signed-off-by: fanchaoting<[email protected]>
Reviewed-by: [email protected]

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




2013-03-27 04:21:15

by fanchaoting

[permalink] [raw]
Subject: [PATCH v1] nfs: nfs client decode fslocations oops if server cheating it

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 <[email protected]>
Reviewed-by: chendt.fnst <[email protected]>

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


2013-03-27 16:17:27

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] nfs: nfs client decode fslocations oops if server cheating it

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
[email protected]
http://www.netapp.com


Attachments:
0001-NFSv4-Fix-Oopses-in-the-fs_locations-code.patch (3.90 kB)
0001-NFSv4-Fix-Oopses-in-the-fs_locations-code.patch