From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "dcritch@redhat.com" <dcritch@redhat.com>,
        "d.lesca@solinos.it" <d.lesca@solinos.it>
Subject: Re: [PATCH 2/2] sunrpc: add bounds checking to svc_rqst_replace_page
Thread-Topic: [PATCH 2/2] sunrpc: add bounds checking to svc_rqst_replace_page
Thread-Index: AQHZWL8ZX7drP5sCN0GjGgDz8G8l567++6EA
Date:   Fri, 17 Mar 2023 13:44:15 +0000
Message-ID: <7DAC68F4-8CE7-4578-BBF1-626285B44B6E@oracle.com>
References: <20230317105608.19393-1-jlayton@kernel.org>
 <20230317105608.19393-2-jlayton@kernel.org>
In-Reply-To: <20230317105608.19393-2-jlayton@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?z8SM2nozYgQKZQO9+//AkW/NHO7MXDqCgUng18aGp3439oKGtAG8kc30H1fj?=
 =?us-ascii?Q?R35dAFeMaBayGIi/Vf7M0j1STQepvdFD6DH0KNmhpBrsJGq1rIurLLPswcwW?=
 =?us-ascii?Q?XqQw0BausuS1BpedOzbgwq4u7H1DrFaBnDOuU6ksi+RuWhfXtfkZxY8ExZu2?=
 =?us-ascii?Q?S0R9LmIn1oDtGFaIaiP/kJ7fVnI1lSV7/UBYPhJhCs0IaM1KVFArTwn4QPiQ?=
 =?us-ascii?Q?/SsgXGbcvR/qxVe30e2swXKVUu6oQrfRsq3kymYrYMsMYBu03EDvyWsK8Ef6?=
 =?us-ascii?Q?Sw8K+NwQdEm5AbrMBszwVGLp+AI06lQrjA9npBX59vBLyDjahdVO1pDDppjl?=
 =?us-ascii?Q?g0JgTszNeKfOzKqOx8UP4h+owlxtwY8Z9JLNYChnfVfFaqUoXmQLt5t1TeUm?=
 =?us-ascii?Q?ip9oJ31eezIvtXCW4nB8IAprApsXSM1Eo0FhnvbYIvSbS3EY79uQJOySExAf?=
 =?us-ascii?Q?CXq0c744xQy0cZ0sfIg4JhsQ3oN74/uJLLFcAxow7Iaota4EIRuT7Ykx1Mn4?=
 =?us-ascii?Q?kmuh+VhVmz+8XsTON7FCYVDkWHEseOv3xQokQ+jn1+zjvaM0n6aOCO1/8jEm?=
 =?us-ascii?Q?401MgvBrcAcpStKPbepEr9mcUEV50q67FM0fAlLiBz4CZjq903q+KXz4l2fk?=
 =?us-ascii?Q?p2E8iMxcfAud/WcgjNbNc82PCtTCGRAmosyD5cgtvE6TN2rCjWZnRjOtWU3W?=
 =?us-ascii?Q?dbHw4mG0Om2Y5oawzqra+BcTSvl0vZwPlcPHyNV8O1jHwrtFmsc5e4iTCyvl?=
 =?us-ascii?Q?7ZQlRIieftxJ6ZTh0Q+fprpeWwio9AqAJZY5TQIfB6xzJ8wJITHyEOdRSjUA?=
 =?us-ascii?Q?fbimLu/et3HJuqPsPsMoLJq9BXfeUemmDDc03gLNfAwBxnR+Ppi3ITJo+ed2?=
 =?us-ascii?Q?KYy4sihuIEMqYEcCwFsNMJSb39w9o+bgq2TOFBM5ZxGjiClMlIxf5k0QOYms?=
 =?us-ascii?Q?9Q9w5xuC4jxXeSutEEpovuGStoaIJcLbzrQYs92fxvP9GaoTSAqWkUYZmbvk?=
 =?us-ascii?Q?tB5UC8l6h8kQhJWDVMXN7PtA4uOpc3PVa9AWpkL6N9yiW892zZ8bOeFr/NnU?=
 =?us-ascii?Q?rryGAABxLAjSlisFt41I0m7sRgd9eMJYGCm4Ur4oEAtIxqOaRorL3BlphNOh?=
 =?us-ascii?Q?5owEml2NGX2wWQW54xRVf1EoGMp41pjeJ8GXjrNXzb+qBzL2SE0IRyUxdqmf?=
 =?us-ascii?Q?wGT8MPYQl14Nw/O+rtEQgEqKycN41xs4YG2sALBPVtpTv21dIi81cZSFrici?=
 =?us-ascii?Q?XTmTQZEHKY5Tb9kwNw9UfuMqs0kydXqmHD3xJPPVfu7P8F1xT4GSp6NoXnNx?=
 =?us-ascii?Q?IkZliuqaRa/XvWC9cEsWYz1hfOq6JPIF76PgPaZjtKDjcZvYa3bYlyEJdGfW?=
 =?us-ascii?Q?MmLKxiKHMdvLy5wqUw84j7irzKIy/PyYTil2fiLy9sviYNLPcCD4DsgDUMA9?=
 =?us-ascii?Q?y1bSnOsT0UYt9axkFOhEyb9VoQklhXMOnik9Mevv1Vqx4CWgvBbzAve3r9Rb?=
 =?us-ascii?Q?b6eJJDnMXOzq+1at8vvpPC/kN94dvGAm3nr626Q19ET2UXHadk/lw0zPh7EN?=
 =?us-ascii?Q?HQWY5/eCFEQzEShhe1V7Bc41l7cqLu1lUKnx+xH9MEjIxrlLsDMQdXu7zVtB?=
 =?us-ascii?Q?4g=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C8F5EEBD947CCC4F96BF7D19896C4BF1@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: 8TwjWR8QAJUrTuJyPmeN1n7V2BSNPsJkDpvieHay2AnCsPB8INlksU4UzATq+rMTVZSlErmzNRx3dpvziHgvMakzuu/Rlp7YgN5ARrRyusmtYXNqHw3B1E4pCkl10srenxx2AMw4ObtSer5H4a2laGLecA4lyRHMjQpVA/2ABWUeBlwhJrnBBkXNfNSEKHEtKT5pOkf1ZPRX1aB+ae6Fwa3D9yPLKGPeA8Vghp4EfkSk0uRHbA8xWvyFlB/29ONOe5DcUS3PDu1pcvHpS4buGlJ0NAhRT+YE+NKQurGj4Ma1rygyms26Sq3Ce1/q540WjWy64PaEc0TQmh5xzcg7257owt7LyHdLV4h8mJfSJ1N46NFCW2pbqp9ngtInjbrfCUhbMlRHmAifdKclrEY9yLCfDiKVI3jnxuhxfEvHxZnePlqsi3kItBfUvCfh/L9QTNLCpOEdBE9ZDzjeHo4v6vpGKLFmz+Nnwn1Xtv297sSQF0IJc2qY7PhCaazBCtxJy4WSE/gNS7HAdAzY6SwWCXg6x1F9X6o7hYujdofWglmz1FiDCHbayZina32Ex0+pWvdvgSDmIivYQl2zahYYptQfKcDfyV7VreCLlk7f+P2SUMl2YetvqbV4JzqxR4/OGojhzjJgK4ZcmH1g6i6YBmv7fTtKxpL2WnFrKnAZ2qCDl9P25SREIzaHg2QgwGHZeDK0ETjXDGEqc1xmSXVnXn2zqMTl9JS9Td1v/4YbowOMXxMqZoUaOl2GK0iPCutI82bPZKRwYYZD8+X/wiWHIZ5FE9CnGc7wX+CEIt1zyrBFouxL7Yz6soN/hXeD2zWgR+szKsGkoxMLcMwJf7Z8QAs1PsYdVlOho31YZk2yvlY=
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: b754b907-db9d-4f48-d3dc-08db26edb2f2
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2023 13:44:15.3782
 (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: MF5Nh8tZErBlYGBsVpYsCMawnes0VBC7vted+pFxFYpAN0BhZzfgNx/wIVFJFyd1zZX7itXh9DjoVQuW9Lrhqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5265
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-17_08,2023-03-16_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 phishscore=0
 suspectscore=0 spamscore=0 adultscore=0 mlxlogscore=731 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303170094
X-Proofpoint-ORIG-GUID: W501uXDEztMogIWJjJNtajO7vYUfzl6x
X-Proofpoint-GUID: W501uXDEztMogIWJjJNtajO7vYUfzl6x
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 17, 2023, at 6:56 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> There's no good way to handle this gracefully, but if rq_next_page ends
> up pointing outside the array, we can at least crash the box before it
> scribbles over too much else.
>=20
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> net/sunrpc/svc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>=20
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index fea7ce8fba14..864e62945647 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -845,6 +845,16 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads);
>  */
> void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
> {
> +	struct page **begin, **end;
> +
> +	/*
> +	 * Bounds check: make sure rq_next_page points into the rq_respages
> +	 * part of the array.
> +	 */
> +	begin =3D rqstp->rq_pages;
> +	end =3D &rqstp->rq_pages[RPCSVC_MAXPAGES];
> +	BUG_ON(rqstp->rq_next_page < begin || rqstp->rq_next_page > end);

Linus has stated clearly that he does not want BUG_ON assertions
if the system is not actually in danger... and this is clearly
the result of a software bug, so a crash will occur anyway.

Can you make this a pr_warn_once() ?


> +
> 	if (*rqstp->rq_next_page) {
> 		if (!pagevec_space(&rqstp->rq_pvec))
> 			__pagevec_release(&rqstp->rq_pvec);
> --=20
> 2.39.2
>=20

--
Chuck Lever