Hi Bruce-
Two clean up patches, and a fix for a SECINFO corner case.
---
Chuck Lever (3):
NFSD: Kill some mixed sign comparisons in fs/nfsd/nfs4xdr.c
NFSD: SECINFO doesn't handle unsupported pseudoflavors correctly
NFSD: Simplify GSS flavor encoding in nfsd4_do_encode_secinfo()
fs/nfsd/nfs4xdr.c | 43 +++++++++++++++++++++++++------------------
fs/nfsd/xdr4.h | 2 +-
2 files changed, 26 insertions(+), 19 deletions(-)
--
Chuck Lever
On Tue, Apr 30, 2013 at 06:48:37PM -0400, Chuck Lever wrote:
> Hi Bruce-
>
> Two clean up patches, and a fix for a SECINFO corner case.
The first two look good, thanks.--b.
>
> ---
>
> Chuck Lever (3):
> NFSD: Kill some mixed sign comparisons in fs/nfsd/nfs4xdr.c
> NFSD: SECINFO doesn't handle unsupported pseudoflavors correctly
> NFSD: Simplify GSS flavor encoding in nfsd4_do_encode_secinfo()
>
>
> fs/nfsd/nfs4xdr.c | 43 +++++++++++++++++++++++++------------------
> fs/nfsd/xdr4.h | 2 +-
> 2 files changed, 26 insertions(+), 19 deletions(-)
>
> --
> Chuck Lever
Clean up.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 9 ++++-----
fs/nfsd/xdr4.h | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 758a1f3..39d60a0 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -244,7 +244,7 @@ static __be32
nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
struct iattr *iattr, struct nfs4_acl **acl)
{
- int expected_len, len = 0;
+ u32 expected_len, len = 0;
u32 dummy32;
char *buf;
int host_err;
@@ -1132,8 +1132,7 @@ nfsd4_decode_verify(struct nfsd4_compoundargs *argp, struct nfsd4_verify *verify
static __be32
nfsd4_decode_write(struct nfsd4_compoundargs *argp, struct nfsd4_write *write)
{
- int avail;
- int len;
+ u32 avail, len;
DECODE_HEAD;
status = nfsd4_decode_stateid(argp, &write->wr_stateid);
@@ -1200,7 +1199,7 @@ static __be32
nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
struct nfsd4_exchange_id *exid)
{
- int dummy, tmp;
+ u32 dummy, tmp;
DECODE_HEAD;
READ_BUF(NFS4_VERIFIER_SIZE);
@@ -3033,7 +3032,7 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd
static __be32
nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_readdir *readdir)
{
- int maxcount;
+ unsigned int maxcount;
loff_t offset;
__be32 *page, *savep, *tailbase;
__be32 *p;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 546f898..c84aea7 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -482,7 +482,7 @@ struct nfsd4_compoundargs {
__be32 * p;
__be32 * end;
struct page ** pagelist;
- int pagelen;
+ unsigned int pagelen;
__be32 tmp[8];
__be32 * tmpp;
struct tmpbuf {
On Tue, Apr 30, 2013 at 06:49:03PM -0400, Chuck Lever wrote:
> Clean up.
I don't want to describe something as cleanup unless it's obvious it
won't change behavior. But:
> @@ -3033,7 +3032,7 @@ nfsd4_encode_readlink(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd
> static __be32
> nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_readdir *readdir)
> {
> - int maxcount;
> + unsigned int maxcount;
> loff_t offset;
> __be32 *page, *savep, *tailbase;
> __be32 *p;
This one, at least, is an actual behavior change. (Note we've got an
"if (maxcount < 0) ..." later on.) Maybe not a real problem, I haven't
checked.
And I haven't checked the others carefully.
--b.
If nfsd4_do_encode_secinfo() can't find GSS info that matches an
export security flavor, it assumes the flavor is not a GSS
pseudoflavor, and simply puts it on the wire.
However, if this XDR encoding logic is given a legitimate GSS
pseudoflavor but the RPC layer says it does not support that
pseudoflavor for some reason, then the server leaks GSS pseudoflavor
numbers onto the wire.
I confirmed this happens by blacklisting rpcsec_gss_krb5, then
attempted a client transition from the pseudo-fs to a Kerberos-only
share. The client received a flavor list containing the Kerberos
pseudoflavor numbers, rather than GSS tuples.
The encoder logic can check that each pseudoflavor in flavs[] is
less than MAXFLAVOR before writing it into the buffer, to prevent
this. But after "nflavs" is written into the XDR buffer, the
encoder can't skip writing flavor information into the buffer when
it discovers the RPC layer doesn't support that flavor.
So count the number of valid flavors as they are written into the
XDR buffer, then write that count into a placeholder in the XDR
buffer when all recognized flavors have been encoded.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ce327e3..758a1f3 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3140,10 +3140,11 @@ static __be32
nfsd4_do_encode_secinfo(struct nfsd4_compoundres *resp,
__be32 nfserr, struct svc_export *exp)
{
- u32 i, nflavs;
+ u32 i, nflavs, supported;
struct exp_flavor_info *flavs;
struct exp_flavor_info def_flavs[2];
- __be32 *p;
+ __be32 *p, *flavorsp;
+ static bool report = true;
if (nfserr)
goto out;
@@ -3167,13 +3168,17 @@ nfsd4_do_encode_secinfo(struct nfsd4_compoundres *resp,
}
}
+ supported = 0;
RESERVE_SPACE(4);
- WRITE32(nflavs);
+ flavorsp = p++; /* to be backfilled later */
ADJUST_ARGS();
+
for (i = 0; i < nflavs; i++) {
+ rpc_authflavor_t pf = flavs[i].pseudoflavor;
struct rpcsec_gss_info info;
- if (rpcauth_get_gssinfo(flavs[i].pseudoflavor, &info) == 0) {
+ if (rpcauth_get_gssinfo(pf, &info) == 0) {
+ supported++;
RESERVE_SPACE(4 + 4 + info.oid.len + 4 + 4);
WRITE32(RPC_AUTH_GSS);
WRITE32(info.oid.len);
@@ -3181,13 +3186,22 @@ nfsd4_do_encode_secinfo(struct nfsd4_compoundres *resp,
WRITE32(info.qop);
WRITE32(info.service);
ADJUST_ARGS();
- } else {
+ } else if (pf < RPC_AUTH_MAXFLAVOR) {
+ supported++;
RESERVE_SPACE(4);
- WRITE32(flavs[i].pseudoflavor);
+ WRITE32(pf);
ADJUST_ARGS();
+ } else {
+ if (report)
+ pr_warn("NFS: SECINFO: security flavor %u "
+ "is not supported\n", pf);
}
}
+ if (nflavs != supported)
+ report = false;
+ *flavorsp = htonl(supported);
+
out:
if (exp)
exp_put(exp);
Clean up.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4xdr.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2502951..ce327e3 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3174,17 +3174,11 @@ nfsd4_do_encode_secinfo(struct nfsd4_compoundres *resp,
struct rpcsec_gss_info info;
if (rpcauth_get_gssinfo(flavs[i].pseudoflavor, &info) == 0) {
- RESERVE_SPACE(4);
+ RESERVE_SPACE(4 + 4 + info.oid.len + 4 + 4);
WRITE32(RPC_AUTH_GSS);
- ADJUST_ARGS();
- RESERVE_SPACE(4 + info.oid.len);
WRITE32(info.oid.len);
WRITEMEM(info.oid.data, info.oid.len);
- ADJUST_ARGS();
- RESERVE_SPACE(4);
WRITE32(info.qop);
- ADJUST_ARGS();
- RESERVE_SPACE(4);
WRITE32(info.service);
ADJUST_ARGS();
} else {