From patchwork Tue May 7 21:49:50 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bruce Fields X-Patchwork-Id: 2536831 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 8BB98DF215 for ; Tue, 7 May 2013 21:49:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759192Ab3EGVtx (ORCPT ); Tue, 7 May 2013 17:49:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15014 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757596Ab3EGVtx (ORCPT ); Tue, 7 May 2013 17:49:53 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r47Lnp2e026643 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 May 2013 17:49:51 -0400 Received: from pad.fieldses.org (vpn-56-201.rdu2.redhat.com [10.10.56.201]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r47Lnpvl013389; Tue, 7 May 2013 17:49:51 -0400 Received: by pad.fieldses.org (Postfix, from userid 2815) id A7DC81A5233; Tue, 7 May 2013 17:49:50 -0400 (EDT) Date: Tue, 7 May 2013 17:49:50 -0400 From: "J. Bruce Fields" To: Simo Sorce Cc: Dan Carpenter , linux-nfs@vger.kernel.org Subject: Re: SUNRPC: Add RPC based upcall mechanism for RPCGSS auth Message-ID: <20130507214950.GD27004@pad.fieldses.org> References: <20130506154435.GA14732@elgon.mountain> <1367855693.2976.205.camel@willson.li.ssimo.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1367855693.2976.205.camel@willson.li.ssimo.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org 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 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 Signed-off-by: J. Bruce Fields --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 #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; }