Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@poochiereds.net>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/4] SUNRPC: Don't leak netobj memory when
 gss_read_proxy_verf() fails
Thread-Topic: [PATCH 1/4] SUNRPC: Don't leak netobj memory when
 gss_read_proxy_verf() fails
Thread-Index: AQHZAdlxKSse4NQqL0iCXWrGsPU+L65UUiYAgAAOMIA=
Date:   Mon, 28 Nov 2022 14:02:16 +0000
Message-ID: <466F83EA-12E1-4C36-8F42-AE4F8578DDEB@oracle.com>
References: <166949601705.106845.10614964159272504008.stgit@klimt.1015granger.net>
 <166949611830.106845.15345645610329421030.stgit@klimt.1015granger.net>
 <3e33c1ba057d39f145bb935b6f92f17dc9cbd207.camel@poochiereds.net>
In-Reply-To: <3e33c1ba057d39f145bb935b6f92f17dc9cbd207.camel@poochiereds.net>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?O1R41y8wshSVVDT7XjuIuHVLDIDbnj1geFbmZuycoycA0OBfGwLx7HfhYCqh?=
 =?us-ascii?Q?gujHlL6wEpGeTlP82lG6xz1sG7PL6HEpmFALyEsPRuwW90xO3vzt5lP9ujRB?=
 =?us-ascii?Q?sQ18Bvwd7/TVxIiNFKLtN8myFjpONQE+UrbIA3UctyitxvS58hFiYoUD/A+9?=
 =?us-ascii?Q?mvJzroi1s3sIxfLpZOCLOoQ64RD1Xjz3OGyfcZh1xTlZF1ZORWkQ7rYAjkzN?=
 =?us-ascii?Q?VHOrhVI2UkljPYjdBMxDMQTzF8gPeAqCHq+jcd8KK0yE6QfqIkNA3sld46/g?=
 =?us-ascii?Q?hIm4nKVNNptZLlg6RGJGqp/QVOXN2vQkARiKOwMqyGgQDAEYyBHobR2u78Hq?=
 =?us-ascii?Q?mmeLcKo4x4t/1RYz726Q5hkpJpyB4D4E3XXDhtRB2YPWIBfnEscVOp5T4n6S?=
 =?us-ascii?Q?SHtrb3KbLJRpQiSXJMf1t2WBu6+b8WdmxE5ylFSdErP8Kz0Nod0wlsIFhJeL?=
 =?us-ascii?Q?hxKkvo+UNPFq06+55qheboqfbfvAO9oP15ZFZCAYsGPuGT0iCKRckCv+jze0?=
 =?us-ascii?Q?WBf0w552Y4H0rbVBmP/BsoY+oGUOn9D+uQ2a1ejiXfB844DxNpDcDK76uetX?=
 =?us-ascii?Q?tQqhOVP2Hdvd2aZHS8ViS0r4BpqBB2IP9KvhY5PIHVmfCKFr3fItwlgotbHc?=
 =?us-ascii?Q?0ZIDuX7KoySoZwAT32BzYtwZmIZrmMFAWwwZyrjR//iULr7FFyuyHXt69rm9?=
 =?us-ascii?Q?ooEBP2lWOQifE+fTNt0hsWSzLID7MRDGmS5I7DDvRm2coB6bDbZ0DA8JfFuE?=
 =?us-ascii?Q?LdcOXvxmuPgRW1rf6n0aFlFoMpxdm0hthQnENJ6bsgDM/Zv8tCGDpI5f/jTK?=
 =?us-ascii?Q?LlR5cBz9KnGq3IHubOcbzmq52Q4SsIY858nzc4CUupVIuDDKVUa9u4anmx/Y?=
 =?us-ascii?Q?/TwdaKQkMe6UlPYO1J8smjUFLy05kbVidN/hlht5OIxbUYs8a5ac7U1K896v?=
 =?us-ascii?Q?G/aQAl9nnxkYbuBVE07LE75ohNcaydI8DtseMPqfQ5GMen498OEAYrHTuZev?=
 =?us-ascii?Q?UPngHdHkzsvGrnLTTD4JrHBdJEg8opxdhBUGzMgYYS6zqV7VCFhPJa+tDLpM?=
 =?us-ascii?Q?I/hkVpmRSgJN4JUQnbYXbmQBo1Y6OKslPE87LEMHqcVe7bcPdMVOiIspuzgX?=
 =?us-ascii?Q?H14hFyjY6B3nsbMIEPnEIw/+7At2jK/A9tTgwdVP8DpUYk2IFLir0jnT0YNu?=
 =?us-ascii?Q?iJlwpz4m44kLmvBuFq/071vOGe6IrF+TOjfn79wnTv8UtPnyCXHuMnXDBOSz?=
 =?us-ascii?Q?7vYsI81qRD3OuYjNij9IUW7X5PGq2eZkJ54UvGNONvom/n1I55OUh7BuxhA3?=
 =?us-ascii?Q?0Qsu7BlqXiwteNIbtVjoZy39MiDtWOyT+gtmRR3iyjLlAAWhmKsO5CK4LaFp?=
 =?us-ascii?Q?FMML2ToOQbWyqXqb72G5G7fKTcMjUSTm9gxkmXN0bhp0qrgaK1myoITe6Gc2?=
 =?us-ascii?Q?W+k3Zcv2v+dkIW24z/7eDUmWDdDyXUtyfqxitpO0N18334ogaa+BUlk/VTlu?=
 =?us-ascii?Q?/aLyRObAGaCbvbvNd4EOfWptO8Woi3rrjbkCPGrKbMhJp+zJi2F5HX1ayZ4K?=
 =?us-ascii?Q?QiDp8aGpzxE6UV6oYNWkEdnA/0hP4dnzjmn0zkdKKwIAYysnHK+t4MAyKH87?=
 =?us-ascii?Q?lQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <B9A757E2DE6F9148ACB453AD47230309@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: cFC2BhcyyY0gTmLugxtmIZG8xD1S5ZZQPwLhBJZAZUnfZpitlq8pg7qq1EUuEUmB8QovZgS1jdiFChrxmSPyxmpbadqngyaF18br9TXNmn9gvOEXRFSk0M/PqRmkYUK8+vbYyHZqE2p04tFc6dLud1bij8Hv4XPm4TGXlMLwEuttH5m7JA2H9Nxe6kH2diigwukiYpUsKbkoMmAJCTe8ROUpx/9KxiM6v84sRQdC1/0fBn+CoORYx87SiCHbrRQmYLc+wUpTCOEzFSltYFkA1YkrCefnHFurneFmykafDFdFwD2gG8K2Iv88TsskftWs0vSxfxk/ancUpT62EwXU8wVqCoUW7fbuq9Eo1v3s6mBoQYwa881/WR7xsymlw3C2YB+kUOq1ZfwKt6yS/tw0Bu8tM7Q35i+G2fHsjK7fe+KVh5LDNO9Iplp/6dJlvKLoNS8AQ6Ij8+FROBKe5e9EtDnpk99Ty+dpN7E86tTl7ECm99n/yRKkSkm9DRxeumauBl/a0shXLteU5yKSer73+SYS++H+2w2B+ICbw7XScCx+TV1XK8arY/c8vSv1wK+zT5nysOkW2SoJcHeoaiT1SKFavL4gJ6xYff7j+FkimcGZdwOV96Hq2GfYiUhDvZxsQcRDKIwRznl/FgkiOqqPL3L7863IrxphBD1y9cPs/mmTvbrmiBhqEP7iJFnt5KgCMUDQKdGAbJsrOqc+vbv7F3PCZDj+mnqT8FNZYRxQiwFgFj1hh0OLEmIJZSjjfFE6fQimBK++1gyqLScYx65/buzYAdqZZuLrRIY8Zl1WCQqrrz18ol6ZlUsk5lIVCR0l
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68b9a45a-3e57-4303-8aca-08dad1492846
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Nov 2022 14:02:16.4251
 (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: XfDd8QvrU4DYSZCQUtXdAoutAxCfUIX/RFR8E7QLsTLiU0hnFBxPl3mPLDsKeKyZRkjeNqjJ3F+5cxEnU9nDSw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5785
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-28_11,2022-11-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 phishscore=0
 mlxlogscore=999 mlxscore=0 adultscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211280107
X-Proofpoint-GUID: CmcH2H8abkaujWtdedAKWQA41P7omr3S
X-Proofpoint-ORIG-GUID: CmcH2H8abkaujWtdedAKWQA41P7omr3S
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Nov 28, 2022, at 8:11 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
>=20
> On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
>> Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS auth=
entication.")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>=20
>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svc=
auth_gss.c
>> index bcd74dddbe2d..9a5db285d4ae 100644
>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> @@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *=
rqstp,
>> 		return res;
>>=20
>> 	inlen =3D svc_getnl(argv);
>> -	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
>> +	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
>> +		kfree(in_handle->data);
>=20
> I wish this were more obvious in this code. It's not at all evident to
> the casual reader that gss_read_common_verf calls dup_netobj here and
> that you need to clean up after it. At a bare minimum, we ought to have
> a comment to that effect over gss_read_common_verf. While you're in
> there, that function is also pretty big to be marked static inline. Can
> you change that too? Ditto for gss_read_verf.

Agreed: I've done that clean up in subsequent patches that are part
of the (yet to be posted) series to replace svc_get/putnl with
xdr_stream.

This seemed like a good fix to apply earlier rather than later. That
should enable it to be backported cleanly.


>> 		return SVC_DENIED;
>> +	}
>>=20
>> 	pages =3D DIV_ROUND_UP(inlen, PAGE_SIZE);
>> 	in_token->pages =3D kcalloc(pages, sizeof(struct page *), GFP_KERNEL);
>> -	if (!in_token->pages)
>> +	if (!in_token->pages) {
>> +		kfree(in_handle->data);
>> 		return SVC_DENIED;
>> +	}
>> 	in_token->page_base =3D 0;
>> 	in_token->page_len =3D inlen;
>> 	for (i =3D 0; i < pages; i++) {
>> 		in_token->pages[i] =3D alloc_page(GFP_KERNEL);
>> 		if (!in_token->pages[i]) {
>> +			kfree(in_handle->data);
>> 			gss_free_in_token_pages(in_token);
>> 			return SVC_DENIED;
>> 		}
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@poochiereds.net>

--
Chuck Lever