Received-SPF: pass (google.com: domain of linux-nfs+bounces-864-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 2 Jan 2024 14:27:17 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Zhipeng Lu <alexious@zju.edu.cn>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>, Jeff Layton <jlayton@kernel.org>,
        Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "J. Bruce Fields" <bfields@fieldses.org>, Simo Sorce <simo@redhat.com>,
        linux-nfs@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [v3] SUNRPC: fix some memleaks in gssx_dec_option_array
Message-ID: <ZZRjlXwc1kIUJKaJ@tissot.1015granger.net>
References: <20240102053815.3611872-1-alexious@zju.edu.cn>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240102053815.3611872-1-alexious@zju.edu.cn>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?hWpKYp0xVZR4hO/6F8TcOFZKgeQWspr3hb84T9/icV4VDln/1uHOe1sFmqOC?=
 =?us-ascii?Q?uqjCJgQ459CjXVkD4FqAUmRxwxGLaBv2zvlB35aS3NdTrZ7NA4fGQo0dQibN?=
 =?us-ascii?Q?Uwy2CZmAQzbIPMZ6P7AHp3LkTnr1QY7mvUMnnkpfO/xm5YA8fZYTAG5/oEvC?=
 =?us-ascii?Q?kXzsUMoajffh46YAZM7eVU/hwv+a3hBZ0G7gRS9Xv89MCcSaNqJ+55vK5orX?=
 =?us-ascii?Q?nQodsVEZ75/0nwYNTuoKOX4vwRx5by7c3Bfk9TC5JgGSLF98EaQj4+Kw5Mgu?=
 =?us-ascii?Q?Igp+llWL8HjJXHdeqL5sszeAMjiCKyu7hA9rpUrrpEId4cxatl5xQYdyfDuk?=
 =?us-ascii?Q?IPbqOn41UdGmf6XpBhY2WVQ89iesDrgUONb1w1OjvX9cPBnAxUMJjIg2sFEF?=
 =?us-ascii?Q?HsxlhaBTclnk6DVxzZaKN6ao/PDAUWz5Spjsa7QxiH+NlQn6kJnFL/j3IxQ/?=
 =?us-ascii?Q?aV/YwSjGVNJRZICIbWD3PYY+hcQCHzPzOWx/S5FBYdwTTv1bhgioyyOcaEQw?=
 =?us-ascii?Q?BbZn8G+x/6maJuSrQa1MRkBBPM6G9wqZV9tqa8BXugSUiqJcApkSxZ3Xni0M?=
 =?us-ascii?Q?/4C0YYY1FvbsXzzXCaMXeLQ8ONsXLFgK1NQMkVyDMPAbJHn1L/4ItttmivN7?=
 =?us-ascii?Q?a6EblhTnpbyz0NEthL2fFfhMV8RfyzJdigqMtqnHgwLE95W/BJDV7bdZvCzb?=
 =?us-ascii?Q?roIeR5WdJfRKMJ3ZvSe5qiP2VFmVWrZ0ETraH6RwW+TCfYWk2cnrPF3K0u8I?=
 =?us-ascii?Q?4TolCuBdus2K6qWTDgN430p5Lu9pctMfv0Uf1MhXs60zq+A+jn4MS999U2hh?=
 =?us-ascii?Q?iL5Nm6sZyvBVMIL81o5IZrKsAfIW/twpGDq5euLUso2PvzWWAHHJ4dpzcmgA?=
 =?us-ascii?Q?EeA1eYO0jNB1XR6E5HZ84HPQgXBOTnbEGxg5/SGitJu6FpKbDogmv9lgQWNK?=
 =?us-ascii?Q?gy+vlofHzYChNokB+MbxbNCt3BdVncS3EJQH7qRcUrauZ+Widben5YKpJ428?=
 =?us-ascii?Q?JMLxvoXdQPdCbguOnqwzDYY0gj9Vyn8xyhpeu+K3O3G9tTW6A66xH3ZkXpUh?=
 =?us-ascii?Q?EKHM/gyVaOdlnaEWLfzbWesyxt57IDHveWgw2rKyJTBKal9FBqOc0ReeSJvO?=
 =?us-ascii?Q?Po5+6XyKYcP0lik9iLrh8DRQrcJPH1d8Iuur1jRNm4r9QGn5ZO4GL1bnYrmy?=
 =?us-ascii?Q?kP5f16hQWbb1in4QludLBjVR+7Qebz1dfOSH+kDlQFSF0PlOfsZugAnVd+E8?=
 =?us-ascii?Q?8DMl4CfsTB+KNimw9OUs3sBMNqCTSwlvNt8XhGnAVu7NFiSrX+FKmKfTFxdX?=
 =?us-ascii?Q?ucFAsfhMaz8HQO8Uc2OVAUl2X4qKl+Pz9dZOxGgRCsXHyWPa6PKUuAuWU/vh?=
 =?us-ascii?Q?5Yf0WKLhc5TDIl4Un5j8DuouQGsmegcBBcOsEp+836eXuSfr5H2P0knlukof?=
 =?us-ascii?Q?nZUOO9rIaVBmTiGRUi4/VbAfUX3OtteKWMrNsingCQ9y3RxbZ5XNv8YKGzJq?=
 =?us-ascii?Q?diedgVC2RnXBmr5IGCCo+OHpDvFA4cz6PTvZ1kZCeBJet7zECBKfIMIK+8ZE?=
 =?us-ascii?Q?b7xM/++Awe0AFXiFYyBVtESYkT1yPMrRr8W/ZR63m4Y6gWfecPszlAQNRQfa?=
 =?us-ascii?Q?FQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	oONDZ5OSKWYJxVwHz5r1AA70jpVv9Fp4BAfpmbV6cjQvLzDXFTqPB+8rV6W8E3kK7DP2+fQcfmmz+HCPcOltPDw8QO4mrtgZJZukclda+YQV004TKqRPsudGE0HlbDEBaa1XecGNV13c8yadVtfL0ege7jxONGaHXsFsTl0dSTQCvHrNTqYip4vUpLrWzWSy3+g3IJErz0COWMatqFXgr1H/7I7NMqD+YVYUDR4IcCS/akjv+aY56z7pTnA3pxvKQYgq27qVkfA7wutVbEttVITTdna9yNUobLtkz/ltJlxEig2IzNFzJm1IDJvnjjpCpFN77WO5x1uVuq1BW0l8JGxLJCohinPTcKkWjJx0Mds+qlEcUQyUu63232KhvYzhgrCzeGHeRSP0dWVSdH9OnrHbQwGdaSgHprCO0rDENdniEoRKujK6t2B5WSoSW8ZRWrjiTpXL/SfkMFAHXp4QpnjmNP8d6pBpea9TGCkbeEQiM/vqpODedcfQeSCSOmwYSrcfHWRRIP9lYh+pgaVbxu2OG5WXBGWqmUkOF14wFOVEL3nIMa31k9vZK5OGwUQk8pmR48V7tcYQDIiH0C6eUNcjH+MZluQ5IhZZoQQWdXk=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: abc78cb9-543f-445b-7b43-08dc0bc8d6d7
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jan 2024 19:27:20.7392
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iUGSlGvisXVxUtzApbUaTBszBceSPrAJ/jUJCdfCU/5bfHhGS6yni++Bne4ljjgXmq48tTY1wXRZsKDb3Bm4/w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7936
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-02_07,2024-01-02_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 malwarescore=0
 adultscore=0 suspectscore=0 mlxscore=0 phishscore=0 mlxlogscore=610
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401020146
X-Proofpoint-GUID: nEMuUVprBzP7HOkJShKhYkFVd3Lg3cVL
X-Proofpoint-ORIG-GUID: nEMuUVprBzP7HOkJShKhYkFVd3Lg3cVL

On Tue, Jan 02, 2024 at 01:38:13PM +0800, Zhipeng Lu wrote:
> The creds and oa->data need to be freed in the error-handling paths after
> there allocation. So this patch add these deallocations in the
> corresponding paths.
> 
> Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
> ---
> Changelog:
> 
> v2: correct some syntactic problems.
> v3: delete unused label err.
> ---
>  net/sunrpc/auth_gss/gss_rpc_xdr.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)

I've applied these two patches for v6.9. They will appear in
nfsd-next once the v6.8 merge window closes.

Another comment below.


> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index d79f12c2550a..cb32ab9a8395 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -250,8 +250,8 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>  
>  	creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
>  	if (!creds) {
> -		kfree(oa->data);
> -		return -ENOMEM;
> +		err = -ENOMEM;
> +		goto free_oa;
>  	}

I recognize that this patch does not introduce memory allocation
in this function. However, the general rule is that XDR decoding
does not perform memory allocation: that is supposed to be handled
by the next layer up.

But I don't see a good reason that memory allocation even has to be
done in here, and in fact both of these allocations are fixed in
size and number. Would it be nicer if these were made fixed fields
in struct gssx_option_array ?

Not an objection to this patch, could be a project for another day.


>  	oa->data[0].option.data = CREDS_VALUE;
> @@ -265,29 +265,40 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>  
>  		/* option buffer */
>  		p = xdr_inline_decode(xdr, 4);
> -		if (unlikely(p == NULL))
> -			return -ENOSPC;
> +		if (unlikely(p == NULL)) {
> +			err = -ENOSPC;
> +			goto free_creds;
> +		}
>  
>  		length = be32_to_cpup(p);
>  		p = xdr_inline_decode(xdr, length);
> -		if (unlikely(p == NULL))
> -			return -ENOSPC;
> +		if (unlikely(p == NULL)) {
> +			err = -ENOSPC;
> +			goto free_creds;
> +		}
>  
>  		if (length == sizeof(CREDS_VALUE) &&
>  		    memcmp(p, CREDS_VALUE, sizeof(CREDS_VALUE)) == 0) {
>  			/* We have creds here. parse them */
>  			err = gssx_dec_linux_creds(xdr, creds);
>  			if (err)
> -				return err;
> +				goto free_creds;
>  			oa->data[0].value.len = 1; /* presence */
>  		} else {
>  			/* consume uninteresting buffer */
>  			err = gssx_dec_buffer(xdr, &dummy);
>  			if (err)
> -				return err;
> +				goto free_creds;
>  		}
>  	}
>  	return 0;
> +
> +free_creds:
> +	kfree(creds);
> +free_oa:
> +	kfree(oa->data);
> +	oa->data = NULL;
> +	return err;
>  }
>  
>  static int gssx_dec_status(struct xdr_stream *xdr,
> -- 
> 2.34.1
> 
> 

-- 
Chuck Lever