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@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] SUNRPC: Fix crasher in unwrap_integ_data()
Thread-Topic: [PATCH] SUNRPC: Fix crasher in unwrap_integ_data()
Thread-Index: AQHZAoQmzavTBIzL1kiUAKK/+ro4Ma5UlBMAgAAAegCAAAstAA==
Date:   Mon, 28 Nov 2022 17:53:55 +0000
Message-ID: <4A7EAC52-1B7E-481B-83A3-53494BD904EB@oracle.com>
References: <166956944745.113279.2771726273440100988.stgit@klimt.1015granger.net>
 <c7ba9e84577d4d1ba917f6f47d1b227e292894ec.camel@kernel.org>
 <c9c934220eadaecea5892ab1e058680ad1369af4.camel@kernel.org>
In-Reply-To: <c9c934220eadaecea5892ab1e058680ad1369af4.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?05gO5S65Q3A9Au/FA0+v/Ps5Pk91XMEWkJAhyyH2cNq7J4rXTKhgJcxijFz1?=
 =?us-ascii?Q?e8L0LueEPfHcaYvJHZ+PymqSTHWxhXsSsogR+8UGfBXVu8YdICfh5bVbuTVB?=
 =?us-ascii?Q?eE2EOv+fzWxMVYLpqob4xez1RuEa1/BP0k8T+tshBktYVSbAu912wwpd8qfo?=
 =?us-ascii?Q?qObvP61Yr66jwCeIVEGnkhbh5FIwX6cPvC5HZSHAjvBNk5li+ujXMR5Yh8qk?=
 =?us-ascii?Q?Cq+gjhvNyRdnShKpExMDj2f8hklBR3kP3kaQMRbKMZl40MtubuoZNG3JAQPq?=
 =?us-ascii?Q?suzdFcKwI84rv/rzW0r9jnRgt+4OXNOTZkSvl6jttYGSAGZ0eQxkue1fhuCX?=
 =?us-ascii?Q?/3jhAkD+pf0A7+BJn1AR7eZXzCop1O1JZ9+TxCEV4aRWdsYH26GOFNrZaxYM?=
 =?us-ascii?Q?cvTVTgeHRQxcmuMJel876znMwFW4CibxjcQ9XreckQLLzx4pfRcm3AGM+iPD?=
 =?us-ascii?Q?9VQ/NJhYtn5/mFVaFbiOwHyPn69s8BLUCe9Xnk8x0Op678lQElb3LIpCcXoj?=
 =?us-ascii?Q?WdB2wrGwAtyNNNm6AYgiVNp7YFFGUozRzwJe/kV8PGkef0EfrtVJmoetdZmE?=
 =?us-ascii?Q?s9XnHax4MfBZRJUWmdB0fymoPV3Yq6ULtrbH+uSxetX1VJujSZE+DDqapeQc?=
 =?us-ascii?Q?dHgxU7QTj3G6FG6VlzvWX1ZbPux6ggLIWTR41PutvsjK7HfBDm3n9OlreQch?=
 =?us-ascii?Q?Ny/E1+2wmgputRBSFnGThhDV522J5bTCk/O+Sed5enBd4zpTc2liOVkGZOxI?=
 =?us-ascii?Q?dLgWFRJhtDwclBSa1F4/BIw9ukf6nZZgMqVbkBjlURExuCJS4DByrIFYFNiO?=
 =?us-ascii?Q?RnIEzeRU+zSY52wxf5f5KwGFn68hhyl0HuS0OpGn+23+mAEJoXUyyDPT4jw6?=
 =?us-ascii?Q?r9XiVHYe718z+VoxSpcfTdo8mqezT38EOwWRT6y5x1NWPyV2mJBua3ine4I4?=
 =?us-ascii?Q?mV9h3gcnP18Psu38ESV+/TMe1mnnrTPumrvW0zbQArCRSuFcLfBUFH4pBSGS?=
 =?us-ascii?Q?F6BN2EW66BbQZfHsXET9JzlBW6x9tVvH8iy7DUU+FsaUJOykl7q7hXvhvcJq?=
 =?us-ascii?Q?RH6zSceokuASILXKqlTd42GTpcaPrK1YkJafQCy5xrPFrkeU1K5lclnxg1MO?=
 =?us-ascii?Q?Y3mVhcn7p9Px4KldNcB5y2EapZNMvV+x3DOjQPkFmMVKglSq1DUhXIdgXUkA?=
 =?us-ascii?Q?ogN19PrgosjRerDrnGF9pXNmpFjaPyMpaRQfHmfLnQc4ZSsrORq00wwrTx0P?=
 =?us-ascii?Q?N6sImXrD54OVQl4rve5D+NdsPV5bZoO+/ICHfQaFw+fhHDd7+GmaErEMg1Pr?=
 =?us-ascii?Q?UcCFKYACOK4+GxBIF/UQ5z/mJ2aPS/peN6mGtOLIj25YxGTYaNmkcKolVu1i?=
 =?us-ascii?Q?YKxt66lzIQ+oZljKEqQuMRmEdMpdOFgtkAqBJXyGKpkk1e7hcDHqZl5xHjdT?=
 =?us-ascii?Q?fJNfdc3Hx8bfRS5HkQYCB1u3ydLQoj8ie48kl1j+ujy6VLOrfm+UhB8IVFDq?=
 =?us-ascii?Q?HEMTgdvg8cgfj7984LMdZ3B2Pr7a3JRr/eApbpMWDVHmUuXUA25tTEsfmgjd?=
 =?us-ascii?Q?cSoxe7vZS/6lw6QJOyu8m/FlPa2IcIA1RneOCPDT0PvfpVhSpUdThFN1t6Jc?=
 =?us-ascii?Q?Cg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <DDB7692712827B4EB7E511E2D93D058E@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: CNm2tm/8WHNeDgyCP9uJdiXIsr7khntfJxNPpAiJzoW1g8/HVsAVp5vn9q5iRsjapvUcD47+huAhPpIOV28UQSeb3UtOnA0vNjgvfnNbYgDEnZsqLzHhhy51JuOV9I4ebuyEkJuPtstmONMceLcc+3cqNRDp2TbjqmWf8InTswFxY6BM55bEQWMKX2GXhtgUSMFHVlv5ToZiDotddl3IMfPxyTBzC9HRE5LcvPrMm7ug299HtJwcuBVyS7QI8e0pT8cJw75bdrOB1miougp6JTxyq5RSaGeBWTZH831K8Zg3fSLNJeRM59prgxx6/08z4sTuDLzVo4tR3O5XArav5zCfDXUtuoi2KfwcqCY4gKZrF0lhDuwNKxZSmQthCyU3rzOvgsrIfiHSG53t4xAhciYfCipfrSToZnTipmj/qwAE11p5t54xE75nj3YABBmg6nYwWC/EMDNlpFRT1pRF+WM6FaeOSPtFsVlWdsOyvNbm+RFEa6EOYjCT7iHwrx54IPdNfHDVgvokRskHBjx/3oZkuZzWkC5gcyXR1i7gMAOLMFYW88z2WW8oqah98e/ruPDq26JMTb+smgI9PxOhAFnve1zSFnrT/5xxe58YCHgvSkkwcGL4G1ESEB0LzFCWhKI0Pxz9wtbk1PU9ZQnZL8F56pYC4Z8z+docxHpHQDRAtT5T5cW74BB2rpZYBN0W6FyduWxno2C5ZGU8PkGz9A+Hm53CkO3AjlnDOjFM9QnvBNMzjOJui8F9EDQotc7L0Q+WdNtkuRozxyM4Fy9dcJAT3EZBXMSFo/JgU4/90N4=
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: a38b2b2c-eeb7-421e-14ba-08dad16984c2
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Nov 2022 17:53:55.4960
 (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: KBfiQI6dsYqDWGr03JMrYGFBL68JL6Pb39I8rPo262tl8k8iBLu4r2avf96GOZ2XA77ttlRzBpOY0+ykbMtOGQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6484
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_15,2022-11-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0 phishscore=0
 mlxscore=0 spamscore=0 adultscore=0 malwarescore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211280131
X-Proofpoint-ORIG-GUID: 8ydj6woXM-HN_QgvXeWV8rNY-2gG6e13
X-Proofpoint-GUID: 8ydj6woXM-HN_QgvXeWV8rNY-2gG6e13
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 12:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-11-28 at 12:12 -0500, Jeff Layton wrote:
>> On Sun, 2022-11-27 at 12:17 -0500, Chuck Lever wrote:
>>> If a zero length is passed to kmalloc() it returns 0x10, which is
>>> not a valid address. gss_verify_mic() subsequently crashes when it
>>> attempts to dereference that pointer.
>>>=20
>>> Instead of allocating this memory on every call based on an
>>> untrusted size value, use a piece of dynamically-allocated scratch
>>> memory that is always available.
>>>=20
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> net/sunrpc/auth_gss/svcauth_gss.c |   55 ++++++++++++++++++++++--------=
-------
>>> 1 file changed, 32 insertions(+), 23 deletions(-)
>>>=20
>>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/sv=
cauth_gss.c
>>> index 9a5db285d4ae..148bb0a7fa5b 100644
>>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>>> @@ -49,11 +49,36 @@
>>> #include <linux/sunrpc/svcauth.h>
>>> #include <linux/sunrpc/svcauth_gss.h>
>>> #include <linux/sunrpc/cache.h>
>>> +#include <linux/sunrpc/gss_krb5.h>
>>>=20
>>> #include <trace/events/rpcgss.h>
>>>=20
>>> #include "gss_rpc_upcall.h"
>>>=20
>>> +/*
>>> + * Unfortunately there isn't a maximum checksum size exported via the
>>> + * GSS API. Manufacture one based on GSS mechanisms supported by this
>>> + * implementation.
>>> + */
>>> +#define GSS_MAX_CKSUMSIZE (GSS_KRB5_TOK_HDR_LEN + GSS_KRB5_MAX_CKSUM_L=
EN)
>>> +
>>> +/*
>>> + * This value may be increased in the future to accommodate other
>>> + * usage of the scratch buffer.
>>> + */
>>> +#define GSS_SCRATCH_SIZE GSS_MAX_CKSUMSIZE
>>> +
>>> +struct gss_svc_data {
>>> +	/* decoded gss client cred: */
>>> +	struct rpc_gss_wire_cred	clcred;
>>> +	/* save a pointer to the beginning of the encoded verifier,
>>> +	 * for use in encryption/checksumming in svcauth_gss_release: */
>>> +	__be32				*verf_start;
>>> +	struct rsc			*rsci;
>>> +
>>> +	/* for temporary results */
>>> +	u8				gsd_scratch[GSS_SCRATCH_SIZE];
>>> +};
>>>=20
>>> /* The rpcsec_init cache is used for mapping RPCSEC_GSS_{,CONT_}INIT re=
quests
>>>  * into replies.
>>> @@ -887,13 +912,11 @@ read_u32_from_xdr_buf(struct xdr_buf *buf, int ba=
se, u32 *obj)
>>> static int
>>> unwrap_integ_data(struct svc_rqst *rqstp, struct xdr_buf *buf, u32 seq,=
 struct gss_ctx *ctx)
>>> {
>>> +	struct gss_svc_data *gsd =3D rqstp->rq_auth_data;
>>> 	u32 integ_len, rseqno, maj_stat;
>>> -	int stat =3D -EINVAL;
>>> 	struct xdr_netobj mic;
>>> 	struct xdr_buf integ_buf;
>>>=20
>>> -	mic.data =3D NULL;
>>> -
>>> 	/* NFS READ normally uses splice to send data in-place. However
>>> 	 * the data in cache can change after the reply's MIC is computed
>>> 	 * but before the RPC reply is sent. To prevent the client from
>>> @@ -917,11 +940,9 @@ unwrap_integ_data(struct svc_rqst *rqstp, struct x=
dr_buf *buf, u32 seq, struct g
>>> 	/* copy out mic... */
>>> 	if (read_u32_from_xdr_buf(buf, integ_len, &mic.len))
>>> 		goto unwrap_failed;
>>> -	if (mic.len > RPC_MAX_AUTH_SIZE)
>>> -		goto unwrap_failed;
>>> -	mic.data =3D kmalloc(mic.len, GFP_KERNEL);
>>> -	if (!mic.data)
>>> +	if (mic.len > sizeof(gsd->gsd_scratch))
>>> 		goto unwrap_failed;
>>> +	mic.data =3D gsd->gsd_scratch;
>>> 	if (read_bytes_from_xdr_buf(buf, integ_len + 4, mic.data, mic.len))
>>> 		goto unwrap_failed;
>>> 	maj_stat =3D gss_verify_mic(ctx, &integ_buf, &mic);
>>> @@ -932,20 +953,17 @@ unwrap_integ_data(struct svc_rqst *rqstp, struct =
xdr_buf *buf, u32 seq, struct g
>>> 		goto bad_seqno;
>>> 	/* trim off the mic and padding at the end before returning */
>>> 	xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
>>> -	stat =3D 0;
>>> -out:
>>> -	kfree(mic.data);
>>> -	return stat;
>>> +	return 0;
>>>=20
>>> unwrap_failed:
>>> 	trace_rpcgss_svc_unwrap_failed(rqstp);
>>> -	goto out;
>>> +	return -EINVAL;
>>> bad_seqno:
>>> 	trace_rpcgss_svc_seqno_bad(rqstp, seq, rseqno);
>>> -	goto out;
>>> +	return -EINVAL;
>>> bad_mic:
>>> 	trace_rpcgss_svc_mic(rqstp, maj_stat);
>>> -	goto out;
>>> +	return -EINVAL;
>>> }
>>>=20
>>> static inline int
>>> @@ -1023,15 +1041,6 @@ unwrap_priv_data(struct svc_rqst *rqstp, struct =
xdr_buf *buf, u32 seq, struct gs
>>> 	return -EINVAL;
>>> }
>>>=20
>>> -struct gss_svc_data {
>>> -	/* decoded gss client cred: */
>>> -	struct rpc_gss_wire_cred	clcred;
>>> -	/* save a pointer to the beginning of the encoded verifier,
>>> -	 * for use in encryption/checksumming in svcauth_gss_release: */
>>> -	__be32				*verf_start;
>>> -	struct rsc			*rsci;
>>> -};
>>> -
>>> static int
>>> svcauth_gss_set_client(struct svc_rqst *rqstp)
>>> {
>>>=20
>>>=20
>>=20
>> That makes a lot more sense!
>>=20
>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>=20
> How did you find this, btw? Is there a bug report or something?

I recently fixed the same problem on the client-side. I managed
to trigger the client-side problem while working on the server
GSS overhaul.


--
Chuck Lever