Hello Simo Sorce,
The patch 1d658336b05f: "SUNRPC: Add RPC based upcall mechanism for
RPCGSS auth" from May 25, 2012, leads to the following warning:
"net/sunrpc/auth_gss/gss_rpc_xdr.c:30 gssx_check_pointer()
warn: signedness bug returning '(-28)'"
net/sunrpc/auth_gss/gss_rpc_xdr.c
24 static bool gssx_check_pointer(struct xdr_stream *xdr)
25 {
26 __be32 *p;
27
28 p = xdr_reserve_space(xdr, 4);
29 if (unlikely(p == NULL))
30 return -ENOSPC;
^^^^^^^
This is casted implicitly to "true". Functions named "check" are a bad
idea anyways because it's not clear what the return value will be. It's
better to use "gssx_pointer_ok()" or "valid" where obviously true means
that the pointer is ok.
31 return *p?true:false;
We just reserved "*p" so doesn't this point to uninitialized data? It
points to a __be32. So we're not really checking a pointer we're
checking that __be32 is non-zero.
32 }
regards,
dan carpenter
On Mon, 2013-05-06 at 18:44 +0300, Dan Carpenter wrote:
> Hello Simo Sorce,
>
> The patch 1d658336b05f: "SUNRPC: Add RPC based upcall mechanism for
> RPCGSS auth" from May 25, 2012, leads to the following warning:
> "net/sunrpc/auth_gss/gss_rpc_xdr.c:30 gssx_check_pointer()
> warn: signedness bug returning '(-28)'"
>
> net/sunrpc/auth_gss/gss_rpc_xdr.c
> 24 static bool gssx_check_pointer(struct xdr_stream *xdr)
> 25 {
> 26 __be32 *p;
> 27
> 28 p = xdr_reserve_space(xdr, 4);
> 29 if (unlikely(p == NULL))
> 30 return -ENOSPC;
> ^^^^^^^
> This is casted implicitly to "true". Functions named "check" are a bad
> idea anyways because it's not clear what the return value will be. It's
> better to use "gssx_pointer_ok()" or "valid" where obviously true means
> that the pointer is ok.
>
> 31 return *p?true:false;
>
> We just reserved "*p" so doesn't this point to uninitialized data? It
> points to a __be32. So we're not really checking a pointer we're
> checking that __be32 is non-zero.
>
> 32 }
>
> regards,
> dan carpenter
Dan,
thanks a lot for pointing this one out.
Bruce,
we should fix it in 3.10 if we can make it.
probably easiest way is to kill lines 29/30 and do:
return (p && *p) ? true : false;
It would be also ok to change the name to gssx_pointer_is_valid() I
guess.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
On Tue, 2013-05-07 at 17:49 -0400, J. Bruce Fields wrote:
> On Mon, May 06, 2013 at 11:54:53AM -0400, Simo Sorce wrote:
> > On Mon, 2013-05-06 at 18:44 +0300, Dan Carpenter wrote:
> > > Hello Simo Sorce,
> > >
> > > The patch 1d658336b05f: "SUNRPC: Add RPC based upcall mechanism for
> > > RPCGSS auth" from May 25, 2012, leads to the following warning:
> > > "net/sunrpc/auth_gss/gss_rpc_xdr.c:30 gssx_check_pointer()
> > > warn: signedness bug returning '(-28)'"
> > >
> > > net/sunrpc/auth_gss/gss_rpc_xdr.c
> > > 24 static bool gssx_check_pointer(struct xdr_stream *xdr)
> > > 25 {
> > > 26 __be32 *p;
> > > 27
> > > 28 p = xdr_reserve_space(xdr, 4);
> > > 29 if (unlikely(p == NULL))
> > > 30 return -ENOSPC;
> > > ^^^^^^^
> > > This is casted implicitly to "true". Functions named "check" are a bad
> > > idea anyways because it's not clear what the return value will be. It's
> > > better to use "gssx_pointer_ok()" or "valid" where obviously true means
> > > that the pointer is ok.
> > >
> > > 31 return *p?true:false;
> > >
> > > We just reserved "*p" so doesn't this point to uninitialized data? It
> > > points to a __be32. So we're not really checking a pointer we're
> > > checking that __be32 is non-zero.
> > >
> > > 32 }
> > >
> > > regards,
> > > dan carpenter
> >
> > Dan,
> > thanks a lot for pointing this one out.
>
> Yes, thanks!
>
> > Bruce,
> > we should fix it in 3.10 if we can make it.
>
> Agreed. Probably the following attempt to decode will fail with an
> -ENOSPC anyway, but this is ugly at least.
>
> > probably easiest way is to kill lines 29/30 and do:
> > return (p && *p) ? true : false;
> >
> > It would be also ok to change the name to gssx_pointer_is_valid() I
> > guess.
>
> It still seems a little yuch to lump a decoding failure with a succesful
> decode of a particular value....
>
> How about just killing gssx_check_pointer, as follows. Admittedly it's
> a bit verbose, but it's straightforward. Any objections?
none if it works the same.
Simo.
> --b.
>
> commit fb43f11c666a4f99f23f0be4fa528dcd288c0da2
> Author: J. Bruce Fields <[email protected]>
> Date: Tue May 7 17:45:20 2013 -0400
>
> SUNRPC: fix decoding of optional gss-proxy xdr fields
>
> The current code works, but sort of by accident: it obviously didn't
> intend the error return to be interpreted as "true".
>
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index a1e1b1a..357f613 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -21,16 +21,6 @@
> #include <linux/sunrpc/svcauth.h>
> #include "gss_rpc_xdr.h"
>
> -static bool gssx_check_pointer(struct xdr_stream *xdr)
> -{
> - __be32 *p;
> -
> - p = xdr_reserve_space(xdr, 4);
> - if (unlikely(p == NULL))
> - return -ENOSPC;
> - return *p?true:false;
> -}
> -
> static int gssx_enc_bool(struct xdr_stream *xdr, int v)
> {
> __be32 *p;
> @@ -802,6 +792,7 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
> struct xdr_stream *xdr,
> struct gssx_res_accept_sec_context *res)
> {
> + u32 value_follows;
> int err;
>
> /* res->status */
> @@ -810,7 +801,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
> return err;
>
> /* res->context_handle */
> - if (gssx_check_pointer(xdr)) {
> + err = gssx_dec_bool(xdr, &value_follows);
> + if (err)
> + return err;
> + if (value_follows) {
> err = gssx_dec_ctx(xdr, res->context_handle);
> if (err)
> return err;
> @@ -819,7 +813,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
> }
>
> /* res->output_token */
> - if (gssx_check_pointer(xdr)) {
> + err = gssx_dec_bool(xdr, &value_follows);
> + if (err)
> + return err;
> + if (value_follows) {
> err = gssx_dec_buffer(xdr, res->output_token);
> if (err)
> return err;
> @@ -828,7 +825,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
> }
>
> /* res->delegated_cred_handle */
> - if (gssx_check_pointer(xdr)) {
> + err = gssx_dec_bool(xdr, &value_follows);
> + if (err)
> + return err;
> + if (value_follows) {
> /* we do not support upcall servers sending this data. */
> return -EINVAL;
> }
--
Simo Sorce * Red Hat, Inc * New York
On Mon, May 06, 2013 at 11:54:53AM -0400, Simo Sorce wrote:
> On Mon, 2013-05-06 at 18:44 +0300, Dan Carpenter wrote:
> > Hello Simo Sorce,
> >
> > The patch 1d658336b05f: "SUNRPC: Add RPC based upcall mechanism for
> > RPCGSS auth" from May 25, 2012, leads to the following warning:
> > "net/sunrpc/auth_gss/gss_rpc_xdr.c:30 gssx_check_pointer()
> > warn: signedness bug returning '(-28)'"
> >
> > net/sunrpc/auth_gss/gss_rpc_xdr.c
> > 24 static bool gssx_check_pointer(struct xdr_stream *xdr)
> > 25 {
> > 26 __be32 *p;
> > 27
> > 28 p = xdr_reserve_space(xdr, 4);
> > 29 if (unlikely(p == NULL))
> > 30 return -ENOSPC;
> > ^^^^^^^
> > This is casted implicitly to "true". Functions named "check" are a bad
> > idea anyways because it's not clear what the return value will be. It's
> > better to use "gssx_pointer_ok()" or "valid" where obviously true means
> > that the pointer is ok.
> >
> > 31 return *p?true:false;
> >
> > We just reserved "*p" so doesn't this point to uninitialized data? It
> > points to a __be32. So we're not really checking a pointer we're
> > checking that __be32 is non-zero.
> >
> > 32 }
> >
> > regards,
> > dan carpenter
>
> Dan,
> thanks a lot for pointing this one out.
Yes, thanks!
> Bruce,
> we should fix it in 3.10 if we can make it.
Agreed. Probably the following attempt to decode will fail with an
-ENOSPC anyway, but this is ugly at least.
> probably easiest way is to kill lines 29/30 and do:
> return (p && *p) ? true : false;
>
> It would be also ok to change the name to gssx_pointer_is_valid() I
> guess.
It still seems a little yuch to lump a decoding failure with a succesful
decode of a particular value....
How about just killing gssx_check_pointer, as follows. Admittedly it's
a bit verbose, but it's straightforward. Any objections?
--b.
commit fb43f11c666a4f99f23f0be4fa528dcd288c0da2
Author: J. Bruce Fields <[email protected]>
Date: Tue May 7 17:45:20 2013 -0400
SUNRPC: fix decoding of optional gss-proxy xdr fields
The current code works, but sort of by accident: it obviously didn't
intend the error return to be interpreted as "true".
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index a1e1b1a..357f613 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -21,16 +21,6 @@
#include <linux/sunrpc/svcauth.h>
#include "gss_rpc_xdr.h"
-static bool gssx_check_pointer(struct xdr_stream *xdr)
-{
- __be32 *p;
-
- p = xdr_reserve_space(xdr, 4);
- if (unlikely(p == NULL))
- return -ENOSPC;
- return *p?true:false;
-}
-
static int gssx_enc_bool(struct xdr_stream *xdr, int v)
{
__be32 *p;
@@ -802,6 +792,7 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
struct xdr_stream *xdr,
struct gssx_res_accept_sec_context *res)
{
+ u32 value_follows;
int err;
/* res->status */
@@ -810,7 +801,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
return err;
/* res->context_handle */
- if (gssx_check_pointer(xdr)) {
+ err = gssx_dec_bool(xdr, &value_follows);
+ if (err)
+ return err;
+ if (value_follows) {
err = gssx_dec_ctx(xdr, res->context_handle);
if (err)
return err;
@@ -819,7 +813,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
}
/* res->output_token */
- if (gssx_check_pointer(xdr)) {
+ err = gssx_dec_bool(xdr, &value_follows);
+ if (err)
+ return err;
+ if (value_follows) {
err = gssx_dec_buffer(xdr, res->output_token);
if (err)
return err;
@@ -828,7 +825,10 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
}
/* res->delegated_cred_handle */
- if (gssx_check_pointer(xdr)) {
+ err = gssx_dec_bool(xdr, &value_follows);
+ if (err)
+ return err;
+ if (value_follows) {
/* we do not support upcall servers sending this data. */
return -EINVAL;
}