Received-SPF: pass (google.com: domain of linux-nfs+bounces-3220-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 9 May 2024 09:19:48 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: 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>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 2/2] NFSD: harden svcxdr_dupstr() and svcxdr_tmpalloc()
 against integer overflows
Message-ID: <ZjzNdLynC7WxwLno@tissot.1015granger.net>
References: <332d1149-988e-4ece-8aef-1e3fb8bf8af4@moroto.mountain>
 <babdb32b-3f3a-4e46-8cbb-26f0ca49cc61@moroto.mountain>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <babdb32b-3f3a-4e46-8cbb-26f0ca49cc61@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?YaolOWEIgw3DJPxJ7eMRutEgQhTWEf+ykmpQ3hqe7TBM1DpfsWa8S3bcKeDB?=
 =?us-ascii?Q?ou74tvHfcVzSpXKC+Xh/39TvkiSxIs3sZIcqc3D1DozgzNP5glCRwrgpbGNT?=
 =?us-ascii?Q?lcRkxf/xjz3RTSGN2HAmPadi6lRanaYm3+mCWQ8t586SAUQXyV/r7AT3h+zE?=
 =?us-ascii?Q?xJ8sUjpTSUL0soIJ30thYD0J1JFAwDd2ZuCz7VxvL+kffh2y/f6+X1x+mZmm?=
 =?us-ascii?Q?qvKpd7E6OvQhyD3wRCWd0rhGDOsfBctvD4hBhRCk+pcyactB0j8AGbhQn3GP?=
 =?us-ascii?Q?aNzFrczm7ROynin9xxUVqPzryEdDBU+7sqeCQdqN0zy2BSOEYYVUWzq86Ifl?=
 =?us-ascii?Q?GM2MQ9eVVHIoAXeV5zo3m1rW5PXgJpgTd2gDIZkUDovP1777X2MkWmuaAvBk?=
 =?us-ascii?Q?N+Bk7guafI218qqXP2eiue6h453nAgqYAGxDEPbtz2zFzRG4gUSsdQi+Z2aX?=
 =?us-ascii?Q?U31J09WijmAB26d/0lggmhS3Wb/U+l2K7hcpOsHTv2vnLTEQ/IgcP6ecVr2c?=
 =?us-ascii?Q?HUOgfb4ctDF1uXOIx/ljONVmemihaMVSuuhC1IT7n/dme3CUqJyOu/kyXG2I?=
 =?us-ascii?Q?naC+hzvmxgjHbW72EgRc2vg/CXavpbvvc58bnkQTfZLNF2umxwY+ApIg7fih?=
 =?us-ascii?Q?U+hXfkO4ns+Gkaexp5hM1oLayoI2aoxduAHhhikQowBlwwqwRxINJShfhYyU?=
 =?us-ascii?Q?ihrs+XMsSuzvBpelhj9QEx9pgbGDNE4yOMnRG82gpPTk0S+d5nz86RhWKu6d?=
 =?us-ascii?Q?aAU7AxBCYvy0dYgfNYrTKdAB8ZJuOnTTWfg8rbKPn0z6Zu27NklzGgCj1nDy?=
 =?us-ascii?Q?h5kAlc/2HTVEbh6vUleTCeWRiwfcQVYxRwTNiqhRRg8Zw2Q04fWkSRDO1kPI?=
 =?us-ascii?Q?+0FUtaLRa2L6aYLR/j+hYP+6QeWtUyOnI+1gJz6xHnbjRTaZTXv4F4uO2jgE?=
 =?us-ascii?Q?SIHMblN9IjNJFuZXqCPK+YRpUlTLDyUwIKPhPdK5exD9oo7qZloHDrHAV7JO?=
 =?us-ascii?Q?ornWCUxCNPI9/BI7A7+DJ9wrn48mAow/S4ZC4kg5YR+SF8jYcKyoV3dmug5D?=
 =?us-ascii?Q?MPsq6HcZ8fxEtw5J6e01XFGJuBKVQ3uGENVJuAiDtiyY9m+lH776O5TCxRK7?=
 =?us-ascii?Q?3UhepH6txcNmnRpjY77IcY5bUHMyKXBA3BRXvUuh9BeulMtQNLDMH5CtcPqu?=
 =?us-ascii?Q?fPRjzD0ZkMZbJl3QnXqhMNDgeGba8YV4Ek+kACbWK0/dzUZeuMpPpI5SDXjO?=
 =?us-ascii?Q?uL2k/eAOk55a/LwJuyffVGXGeAZYcxD6HnrbGxjulBfB0fMv2lYi08zPOocr?=
 =?us-ascii?Q?/dWEBdn529Xj4mxOc8IIBApBizZ0hFsscV1/djBTtV0GXsng9DBFXWtC3AoV?=
 =?us-ascii?Q?MVSTtatIK5xPRrfQjSD8MVe7bZT7N0H5eOukjfMlBwPuo2OdWNRy5ozL/M1y?=
 =?us-ascii?Q?77K/X88KZ/QcC205cpdcCltlhtPf/jtu+ocaPQYcuZPzR1YyhxvBVRzaOOnN?=
 =?us-ascii?Q?toxA306P/N+gKbk73uviSXpva6fdULwd3sQFWGSb6i9oYrX8Z9Pl6us5Jnuz?=
 =?us-ascii?Q?lTmf0pyuYO1sl8tz+AutIOMazHaoGI60T+VPdlfVbe5KxqLdx/zYehPsxaxl?=
 =?us-ascii?Q?Gw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	KRz1B5iScJp9Q1EV0EDZBnhzzKkgwK+TEnPH+ECIt2V7if0UGDWTVz16khu7HKNgtYOjhxUZHJSavXCTf/q/zyWeIkih1EAMxfprQsbOo475qGkHsmgHEDwvrXeSnYzeczf930X+x4KTYl3xGQy0QULj7kyvPobe2C8gQ7gSucn3O7MVdZrEwajAJrusqGAfVNxiI6yETnrFN03qfR9QOMeTAfDujKyLfPuoY5P/Qeia6/x6de4njQCE3nK1byFzzJ/S9sQet9Ea8HLTz3njdNaCfFMRGh5u472ZJBFbZPuea5M6OT4afYly0KxlIoxWq7gB+9TR6DwDgFPKN36c5PzQtbVVrsueU9HYsCfgN+nri8MIY/87gNq/VLTRNODPlZbUga48BPfozclL5YLvhh9qY7uDn00HCrzPgwCF/U+s7e02YSHCjnqtc3QTYciknBIdkBBjc5wHI4vGa9st8LRM35CC1r20q6m3fzgHwVj3tvC+VxTO5EV0WKia+m6oWOtkMR42K5ra0txjjVhDGWEyGHQVPxRyK8eWCgqyBhCN/iD2o1FVK0iFim4jdIG5vt6xZriSoBSzwQcRncxNegx4Iw6WPOKPb6IbAcj/hug=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 51cf4864-7ed5-4bfd-8c1d-08dc702ab566
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 13:19:51.6083
 (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: eIJfpM072V1BzTfnn+ikpINlFKFupNDsQwOcLTXgm4RBQBDu7iw8aTeWkQ65lwc0cVMYAR1RWH36+HgrbADjTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7678
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-09_06,2024-05-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 mlxscore=0
 suspectscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2405090089
X-Proofpoint-GUID: 2qwr-H2iapIqsqmVZXe9U5cvhQoVP4jK
X-Proofpoint-ORIG-GUID: 2qwr-H2iapIqsqmVZXe9U5cvhQoVP4jK

On Thu, May 09, 2024 at 01:48:28PM +0300, Dan Carpenter wrote:
> These lengths come from xdr_stream_decode_u32() and so we should be a
> bit careful with them.  Use size_add() and struct_size() to avoid
> integer overflows.  Saving size_add()/struct_size() results to a u32 is
> unsafe because it truncates away the high bits.
> 
> Also generally storing sizes in longs is safer.  Most systems these days
> use 64 bit CPUs.  It's harder for an addition to overflow 64 bits than
> it is to overflow 32 bits.  Also functions like vmalloc() can
> successfully allocate UINT_MAX bytes, but nothing can allocate ULONG_MAX
> bytes.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I think my patch 1 fixes any real issues.  It's hard to assign a Fixes
> tag to this.

I agree that this is a defensive change only. As it is late in the
cycle and this doesn't seem urgent, I would prefer to queue this
change for v6.11.


>  fs/nfsd/nfs4xdr.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c7bfd2180e3f..42b41d55d4ed 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -118,11 +118,11 @@ static int zero_clientid(clientid_t *clid)
>   * operation described in @argp finishes.
>   */
>  static void *
> -svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
> +svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, size_t len)
>  {
>  	struct svcxdr_tmpbuf *tb;
>  
> -	tb = kmalloc(sizeof(*tb) + len, GFP_KERNEL);
> +	tb = kmalloc(struct_size(tb, buf, len), GFP_KERNEL);
>  	if (!tb)
>  		return NULL;
>  	tb->next = argp->to_free;
> @@ -138,9 +138,9 @@ svcxdr_tmpalloc(struct nfsd4_compoundargs *argp, u32 len)
>   * buffer might end on a page boundary.
>   */
>  static char *
> -svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
> +svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, size_t len)
>  {
> -	char *p = svcxdr_tmpalloc(argp, len + 1);
> +	char *p = svcxdr_tmpalloc(argp, size_add(len, 1));
>  
>  	if (!p)
>  		return NULL;
> @@ -150,7 +150,7 @@ svcxdr_dupstr(struct nfsd4_compoundargs *argp, void *buf, u32 len)
>  }
>  
>  static void *
> -svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, u32 len)
> +svcxdr_savemem(struct nfsd4_compoundargs *argp, __be32 *p, size_t len)
>  {
>  	__be32 *tmp;
>  
> @@ -2146,7 +2146,7 @@ nfsd4_decode_clone(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
>   */
>  static __be32
>  nfsd4_vbuf_from_vector(struct nfsd4_compoundargs *argp, struct xdr_buf *xdr,
> -		       char **bufp, u32 buflen)
> +		       char **bufp, size_t buflen)
>  {
>  	struct page **pages = xdr->pages;
>  	struct kvec *head = xdr->head;
> -- 
> 2.43.0
> 

-- 
Chuck Lever