These are some (what look like mostly minor) fixes prompted by some
static checker results from Andi Kleen. I'm planning to queue them up
for 3.13.
--b.
From: "J. Bruce Fields" <[email protected]>
A couple times recently somebody has noticed that we're ignoring a
sequence number here and wondered whether there's a bug.
In fact, there's not. Thanks to Andy Adamson for pointing out a useful
explanation in rfc 2203. Add comments citing that rfc, and remove
"seqnum" to prevent static checkers complaining about unused variables.
Reported-by: Andi Kleen <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/auth_gss/gss_krb5_unseal.c | 8 ++++----
net/sunrpc/auth_gss/gss_krb5_wrap.c | 6 ++++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_krb5_unseal.c b/net/sunrpc/auth_gss/gss_krb5_unseal.c
index 6cd930f..6c981dd 100644
--- a/net/sunrpc/auth_gss/gss_krb5_unseal.c
+++ b/net/sunrpc/auth_gss/gss_krb5_unseal.c
@@ -150,7 +150,6 @@ gss_verify_mic_v2(struct krb5_ctx *ctx,
struct xdr_netobj cksumobj = {.len = sizeof(cksumdata),
.data = cksumdata};
s32 now;
- u64 seqnum;
u8 *ptr = read_token->data;
u8 *cksumkey;
u8 flags;
@@ -197,9 +196,10 @@ gss_verify_mic_v2(struct krb5_ctx *ctx,
if (now > ctx->endtime)
return GSS_S_CONTEXT_EXPIRED;
- /* do sequencing checks */
-
- seqnum = be64_to_cpup((__be64 *)ptr + 8);
+ /*
+ * NOTE: the sequence number at ptr + 8 is skipped, rpcsec_gss
+ * doesn't want it checked; see page 6 of rfc 2203.
+ */
return GSS_S_COMPLETE;
}
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 1da52d1..5040a46 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -489,7 +489,6 @@ static u32
gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
{
s32 now;
- u64 seqnum;
u8 *ptr;
u8 flags = 0x00;
u16 ec, rrc;
@@ -525,7 +524,10 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
ec = be16_to_cpup((__be16 *)(ptr + 4));
rrc = be16_to_cpup((__be16 *)(ptr + 6));
- seqnum = be64_to_cpup((__be64 *)(ptr + 8));
+ /*
+ * NOTE: the sequence number at ptr + 8 is skipped, rpcsec_gss
+ * doesn't want it checked; see page 6 of rfc 2203.
+ */
if (rrc != 0)
rotate_left(offset + 16, buf, rrc);
--
1.7.9.5
From: "J. Bruce Fields" <[email protected]>
Reported-by: Andi Kleen <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/auth_gss/gss_rpc_xdr.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index f0f78c5..1ec19f6 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -559,6 +559,8 @@ static int gssx_enc_cred(struct xdr_stream *xdr,
/* cred->elements */
err = dummy_enc_credel_array(xdr, &cred->elements);
+ if (err)
+ return err;
/* cred->cred_handle_reference */
err = gssx_enc_buffer(xdr, &cred->cred_handle_reference);
@@ -740,22 +742,20 @@ void gssx_enc_accept_sec_context(struct rpc_rqst *req,
goto done;
/* arg->context_handle */
- if (arg->context_handle) {
+ if (arg->context_handle)
err = gssx_enc_ctx(xdr, arg->context_handle);
- if (err)
- goto done;
- } else {
+ else
err = gssx_enc_bool(xdr, 0);
- }
+ if (err)
+ goto done;
/* arg->cred_handle */
- if (arg->cred_handle) {
+ if (arg->cred_handle)
err = gssx_enc_cred(xdr, arg->cred_handle);
- if (err)
- goto done;
- } else {
+ else
err = gssx_enc_bool(xdr, 0);
- }
+ if (err)
+ goto done;
/* arg->input_token */
err = gssx_enc_in_token(xdr, &arg->input_token);
@@ -763,13 +763,12 @@ void gssx_enc_accept_sec_context(struct rpc_rqst *req,
goto done;
/* arg->input_cb */
- if (arg->input_cb) {
+ if (arg->input_cb)
err = gssx_enc_cb(xdr, arg->input_cb);
- if (err)
- goto done;
- } else {
+ else
err = gssx_enc_bool(xdr, 0);
- }
+ if (err)
+ goto done;
err = gssx_enc_bool(xdr, arg->ret_deleg_cred);
if (err)
--
1.7.9.5
On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> For every other problem here we bail out with an error, but here for
> some reason we're setting a negative cache entry (with, note, an
> undefined expiry).
>
> It seems simplest just to bail out in the same way as we do in other
> cases.
>
> Cc: Simo Sorce <[email protected]>
> Reported-by: Andi Kleen <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 09fb638..008cdad 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
> if (!ud->found_creds) {
> /* userspace seem buggy, we should always get at least a
> * mapping to nobody */
> - dprintk("RPC: No creds found, marking Negative!\n");
> - set_bit(CACHE_NEGATIVE, &rsci.h.flags);
> + dprintk("RPC: No creds found!\n");
> + goto out;
> } else {
>
> /* steal creds */
IIRC, we are doing this to avoid rapid upcall loops in the kernel, where
we keep hammering upcalls out and keep getting an error back.
I am not sure it is wise to not set a temp negative cache here.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
On Thu, 2013-10-10 at 15:23 -0400, J. Bruce Fields wrote:
> On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote:
> > On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <[email protected]>
> > >
> > > For every other problem here we bail out with an error, but here for
> > > some reason we're setting a negative cache entry (with, note, an
> > > undefined expiry).
> > >
> > > It seems simplest just to bail out in the same way as we do in other
> > > cases.
> > >
> > > Cc: Simo Sorce <[email protected]>
> > > Reported-by: Andi Kleen <[email protected]>
> > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > ---
> > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > > index 09fb638..008cdad 100644
> > > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
> > > if (!ud->found_creds) {
> > > /* userspace seem buggy, we should always get at least a
> > > * mapping to nobody */
> > > - dprintk("RPC: No creds found, marking Negative!\n");
> > > - set_bit(CACHE_NEGATIVE, &rsci.h.flags);
> > > + dprintk("RPC: No creds found!\n");
> > > + goto out;
> > > } else {
> > >
> > > /* steal creds */
> >
> > IIRC, we are doing this to avoid rapid upcall loops in the kernel, where
> > we keep hammering upcalls out and keep getting an error back.
>
> Looks like returning an error instead results in closing the connection
> to the client, so, depends how the client replies I guess.
>
> In any case I don't see why we'd treat this particular gss-proxy bug
> differently than we would any other (like, say, passing down bad xdr, or
> a gss context that we can't import).
Uhmm I did not recall that, I guess the change is ok then.
Simo.
> --b.
>
> > I am not sure it is wise to not set a temp negative cache here.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Simo Sorce * Red Hat, Inc * New York
From: "J. Bruce Fields" <[email protected]>
We depend on the xdr decoder to set this pointer, but if we error out
before we decode this piece it could be left NULL.
I think this is probably tough to hit without a buggy gss-proxy.
Reported-by: Andi Kleen <[email protected]>
Cc: Simo Sorce <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/auth_gss/gss_rpc_upcall.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
index f1eb0d1..458f85e 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
@@ -298,7 +298,8 @@ int gssp_accept_sec_context_upcall(struct net *net,
if (res.context_handle) {
data->out_handle = rctxh.exported_context_token;
data->mech_oid.len = rctxh.mech.len;
- memcpy(data->mech_oid.data, rctxh.mech.data,
+ if (rctxh.mech.data)
+ memcpy(data->mech_oid.data, rctxh.mech.data,
data->mech_oid.len);
client_name = rctxh.src_name.display_name;
}
--
1.7.9.5
On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> We depend on the xdr decoder to set this pointer, but if we error out
> before we decode this piece it could be left NULL.
>
> I think this is probably tough to hit without a buggy gss-proxy.
>
> Reported-by: Andi Kleen <[email protected]>
> Cc: Simo Sorce <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> net/sunrpc/auth_gss/gss_rpc_upcall.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> index f1eb0d1..458f85e 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> @@ -298,7 +298,8 @@ int gssp_accept_sec_context_upcall(struct net *net,
> if (res.context_handle) {
> data->out_handle = rctxh.exported_context_token;
> data->mech_oid.len = rctxh.mech.len;
> - memcpy(data->mech_oid.data, rctxh.mech.data,
> + if (rctxh.mech.data)
> + memcpy(data->mech_oid.data, rctxh.mech.data,
> data->mech_oid.len);
> client_name = rctxh.src_name.display_name;
> }
Reviewed-by: Simo Sorce <[email protected]>
--
Simo Sorce * Red Hat, Inc * New York
From: "J. Bruce Fields" <[email protected]>
For every other problem here we bail out with an error, but here for
some reason we're setting a negative cache entry (with, note, an
undefined expiry).
It seems simplest just to bail out in the same way as we do in other
cases.
Cc: Simo Sorce <[email protected]>
Reported-by: Andi Kleen <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 09fb638..008cdad 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
if (!ud->found_creds) {
/* userspace seem buggy, we should always get at least a
* mapping to nobody */
- dprintk("RPC: No creds found, marking Negative!\n");
- set_bit(CACHE_NEGATIVE, &rsci.h.flags);
+ dprintk("RPC: No creds found!\n");
+ goto out;
} else {
/* steal creds */
--
1.7.9.5
On Thu, Oct 10, 2013 at 04:45:52PM -0400, Simo Sorce wrote:
> On Thu, 2013-10-10 at 15:23 -0400, J. Bruce Fields wrote:
> > On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote:
> > > On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <[email protected]>
> > > >
> > > > For every other problem here we bail out with an error, but here for
> > > > some reason we're setting a negative cache entry (with, note, an
> > > > undefined expiry).
> > > >
> > > > It seems simplest just to bail out in the same way as we do in other
> > > > cases.
> > > >
> > > > Cc: Simo Sorce <[email protected]>
> > > > Reported-by: Andi Kleen <[email protected]>
> > > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > > ---
> > > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > > > index 09fb638..008cdad 100644
> > > > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > > > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
> > > > if (!ud->found_creds) {
> > > > /* userspace seem buggy, we should always get at least a
> > > > * mapping to nobody */
> > > > - dprintk("RPC: No creds found, marking Negative!\n");
> > > > - set_bit(CACHE_NEGATIVE, &rsci.h.flags);
> > > > + dprintk("RPC: No creds found!\n");
> > > > + goto out;
> > > > } else {
> > > >
> > > > /* steal creds */
> > >
> > > IIRC, we are doing this to avoid rapid upcall loops in the kernel, where
> > > we keep hammering upcalls out and keep getting an error back.
> >
> > Looks like returning an error instead results in closing the connection
> > to the client, so, depends how the client replies I guess.
> >
> > In any case I don't see why we'd treat this particular gss-proxy bug
> > differently than we would any other (like, say, passing down bad xdr, or
> > a gss context that we can't import).
>
> Uhmm I did not recall that, I guess the change is ok then.
OK, thanks for the review.
--b.
On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote:
> On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <[email protected]>
> >
> > For every other problem here we bail out with an error, but here for
> > some reason we're setting a negative cache entry (with, note, an
> > undefined expiry).
> >
> > It seems simplest just to bail out in the same way as we do in other
> > cases.
> >
> > Cc: Simo Sorce <[email protected]>
> > Reported-by: Andi Kleen <[email protected]>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 09fb638..008cdad 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
> > if (!ud->found_creds) {
> > /* userspace seem buggy, we should always get at least a
> > * mapping to nobody */
> > - dprintk("RPC: No creds found, marking Negative!\n");
> > - set_bit(CACHE_NEGATIVE, &rsci.h.flags);
> > + dprintk("RPC: No creds found!\n");
> > + goto out;
> > } else {
> >
> > /* steal creds */
>
> IIRC, we are doing this to avoid rapid upcall loops in the kernel, where
> we keep hammering upcalls out and keep getting an error back.
Looks like returning an error instead results in closing the connection
to the client, so, depends how the client replies I guess.
In any case I don't see why we'd treat this particular gss-proxy bug
differently than we would any other (like, say, passing down bad xdr, or
a gss context that we can't import).
--b.
> I am not sure it is wise to not set a temp negative cache here.