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++6EAgAACY4CAAACSgA==
Date:   Fri, 17 Mar 2023 13:54:49 +0000
Message-ID: <29DB4C06-CB3F-4076-AB90-ABE61A2AEC27@oracle.com>
References: <20230317105608.19393-1-jlayton@kernel.org>
 <20230317105608.19393-2-jlayton@kernel.org>
 <7DAC68F4-8CE7-4578-BBF1-626285B44B6E@oracle.com>
 <ca552165dc70f8268b887bc35d395039ed093861.camel@kernel.org>
In-Reply-To: <ca552165dc70f8268b887bc35d395039ed093861.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?C+K+hdl5r6nk6hnG5QHeSfT1BpKliuf+CpGTDYFE1ZWF8Pq7flT4nLDxrO6L?=
 =?us-ascii?Q?D9pR39jFArX7wvKstL0ZaTdvuk1SjbwyYhZMbwGOeMaL/I3ucEPvfUUOntnp?=
 =?us-ascii?Q?pbrykJsAyiU+emQwbXmgYDSONJICsgOtXeTEh+3tsB/JD9i3X9qUZo+uIb3U?=
 =?us-ascii?Q?wEx2jZCeDHA4x9hTcH16AwwzgPm70TL0gYdq5QdifI1wvwN1D3VAdJD+GITv?=
 =?us-ascii?Q?5r/ExbTR0yf2ypa2fV38rZZ9AZRSqLyb3dUBY/mXh5ov1J5XDt3MIYmaNnlQ?=
 =?us-ascii?Q?drsmzApBNFug/uG3g8YuFFMD2NP0iJXQDuVB+zocjFyMatXYAHkOyZuC7MXm?=
 =?us-ascii?Q?6M5pwGzDRdSPxOvOFsPS5hfBepfafCQ3AMjWSOp2eB+d9dHX++TWz0rEVrqE?=
 =?us-ascii?Q?X3r91MmekLsoVR0bGqZc5gcyjC9fRMr9mbZfs24bMz0uw+vBQdvZhXPkmlYB?=
 =?us-ascii?Q?lzHiWWjMAhi0pro/wyU66ZX0gy5XiZVl2VzAccrQ0y0nwVC5q/oOYoybvJfV?=
 =?us-ascii?Q?159UcXpBPZNOLXMmF9H1wWakLzJAJR1ltDUPIDV8DPV8zJvuTvORDuT85ZRu?=
 =?us-ascii?Q?5HcQpvP8GqPP4k66xPNQ3Mu00IqS4S6vQrdSqK7gHpQeq306oilhb5X9IxJx?=
 =?us-ascii?Q?0It/MhmsEAuReg4kr9TdrN8PctC66NqI+u5/Xx3aSgPSeNJBg7pdHLA0CIxC?=
 =?us-ascii?Q?E4wx3aX+LExcwOj7W5f4gBVfOCAUzjsHk+K1Psf94D/kevhDCTnLIWrWC//f?=
 =?us-ascii?Q?7EZVyvR2sDTG4UyO9jRZBgyE7+PGRupVof0haTNQ5/fIaUNKLRbCK5x/EAp9?=
 =?us-ascii?Q?l8KqhIVYsUHooPy+NvBHOMeX/8yUpaQiREiKG/8vMBhe+/1Sg5ZW69t1odP5?=
 =?us-ascii?Q?Rvk+uu17BQDnGnWseWwzldaJgpK3nwCnqKaED59Bx7OEmBFHm2RVKRQZZxid?=
 =?us-ascii?Q?R5W6bTPGff5V2RzfgQ5IDhAWyyNYeoip/dedozaTiT0cbd91hFp7AHXGQq7T?=
 =?us-ascii?Q?ChutfxKguy5HGrg/WvY8CKclCKp2wsd/+jl+0exkpp+Mvw/+dvypQL22YcY1?=
 =?us-ascii?Q?de5d2gcriAT3zTlcU1JmH14SChhRKrMrAyDJtHRCpXZGjyVAkP63qkVbzPcu?=
 =?us-ascii?Q?V2v6ZgCTlOSEeXIaLVyYqILeJR4PedGraQ2mCPSw9ZFhtCXULgL6Hq2JBtZE?=
 =?us-ascii?Q?1nwnH2J3QGScWvxOisIDle7uKMp6FJvAkLvI/uGKl1MqizKNYyFxo2lXr5To?=
 =?us-ascii?Q?ZrPy+V1qvvy07dwNxLvK/CuKwZmwgcGCu6yDckkdLPOZibrhFeXPSIetSLY1?=
 =?us-ascii?Q?p/tHeOkbC4e81OCbonSQ/pM6rpyypjqGpmaUwQ8fawRP9pVx741p33/6YLqR?=
 =?us-ascii?Q?BYA4NByLhzkPJAlDXIsIrddL2Qtp/vDvt4/TvHySMCoCgl8wrkAWk/UWauy/?=
 =?us-ascii?Q?SVamKwTyEpOyQhnGLhNERQC9sr/oF3CwKrdXsduotpL8bqijt7XQ6hUCsTU9?=
 =?us-ascii?Q?BLGAjOCEkPSEa+PjjI+O+UkBLig0YHSI29uyFxpDRQZiexbbbp4VvTah6Ib+?=
 =?us-ascii?Q?bqWbiShaeGIHWlMZxNnAZSKVlSsdfL/IZQjPNH0XRMLrMQWJe++dky3rdYLF?=
 =?us-ascii?Q?Bw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <A2E3D00835722549A67BF59E96A72D98@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: q/BKnmwApi8pgb474rLXrnionJ1bUvtVEiNq0rMSXclwe7mQ9hrLktSImD58Hk7sENxp236IdR7Qd38Rv26f5RIJXR56Mtn8hNkLCbuVKfTGAUylhlI57cjJoAbu9se0WigIgGJ6GtoYeLwSiAHCUdNVoSjFFCp1GlzmZ9lXT1IcsUYd+UaOUVJgCui29yrIEZewM49Wbclcc/o5mh43u0LhI6FMPn+CShAOzdyElG7e/YD4ed1zIEs5KGr/+44rwSNXIeMVhiRN7WCjtFuqhqjl8qwBYPEZqJUkFEmhzNIqD7SlRs193DPIkrclDqAtRG5PCtWe1dKxlna/ykxF/LlynK66BdTJORLjqefSFXa7/kkBMLPzkiYnib85N3wU9rNrpkIG+3iRJ/Xh1FWgaMgQbCUqCdbusiFNnq9YAIdjiQIp7CtC0kmh5wSMYZXQQiBaq99MhDi6bZxvYrJntUJYXxxBl1htRPYZ5HtXcLnBgwhg6lBWP94RY0NH2DBTexVxHZ/phwP20erBVvH+B8EKN8AW9lEAzlXyd4+SbwtSzD5597yKLx5awCx0Fa3Eu+XpZ0muM0MqeTYuxHfxjg2avppqxB2VMyA8/MvL2yOz2iVvY3gCP1LZdsXiZByNQwPNlLQUecJ8KBBKhgW4C6/AH58SAIClbMi8lhq8X9JtQz+eDG1OnUPq4/UA/d3LGU2Mw8xk/2yKuJvSJeCGy+TxLuJSUhQVCc0WQ8MVfgQnR0V7DymN832bX6Tmp8zbjS0Y/okYtkFuV0lj4b7cSh5W/BpnG6JvUGw4j49NgdFV2tBcT25/q4yrcOx8BVUTU+bwYTLqkSrzdTu5QmKJ5zvQ1T2bZKdy5IOzcQ7XXQU=
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: 4601e091-7e8b-4a76-e767-08db26ef2cea
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2023 13:54:49.5532
 (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: cke2z25+L/z/lhr7755KFAuxVVy/aWJXAJ40MbU0HJ0zyVE9begWup8YJvzDt1O3FpRSQjAhacA8x2kFa84BUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4173
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 spamscore=0 phishscore=0
 mlxlogscore=920 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303150002 definitions=main-2303170095
X-Proofpoint-ORIG-GUID: KX5aMO0QBRbc9MGkkegNKhBJLr2xlfHo
X-Proofpoint-GUID: KX5aMO0QBRbc9MGkkegNKhBJLr2xlfHo
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 17, 2023, at 9:52 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2023-03-17 at 13:44 +0000, Chuck Lever III wrote:
>>=20
>>> 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);
>>=20
>> 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.
>>=20
>=20
> It'll crash, but only after we scribble over some memory.
>=20
> Actually, it looks like the splice actor can return an error. We could
> return -EIO here or something without doing anything if we hit this case
> and then let that bubble back up to the read?

Yes, if it's possible to fail just the READ operation, that
would be best. Maybe a emitting a trace event would be better
than a pr_warn.


>> Can you make this a pr_warn_once() ?
>>=20
>>=20
>>> +
>>> 	if (*rqstp->rq_next_page) {
>>> 		if (!pagevec_space(&rqstp->rq_pvec))
>>> 			__pagevec_release(&rqstp->rq_pvec);
>>> --=20
>>> 2.39.2
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever