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:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Jan Kasiak <j.kasiak@gmail.com>
Subject: Re: [PATCH] lockd: detect and reject lock arguments that overflow
Thread-Topic: [PATCH] lockd: detect and reject lock arguments that overflow
Thread-Index: AQHYpcq2MEaCGFN5/EyCMrMShco2TK2aa1SA
Date:   Mon, 1 Aug 2022 19:18:20 +0000
Message-ID: <1DB7763E-BE7A-4123-8AC4-E8D154C07DE3@oracle.com>
References: <20220801171819.125532-1-jlayton@kernel.org>
In-Reply-To: <20220801171819.125532-1-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?j3DfiEXifj77Zx6TNRsLm6oiOM2I0LGqdSEaioRpQY17OylfCce6c2FQKl2D?=
 =?us-ascii?Q?qG0eVsamVOG9QR9LccRzVl3uQtYH5zhNm/wguscY7Myc7ORBAzyResIfGjQ9?=
 =?us-ascii?Q?UFitElPZsmLaq3GdCIqXU7tz/mW1YmN4ToiTRf1dlQ/3prESDVxkr5gsQq7U?=
 =?us-ascii?Q?AqLzGK2uX+5sHJYR/0tLNysxpHYRwhSJX/JPYuNRPUl2Lz/mCpI8tJ/thppK?=
 =?us-ascii?Q?o8HWQv6csts16nMnbxONGsazJHKBDkqcd0vX/IQAl2PNxYB90pNaUUSCRO3B?=
 =?us-ascii?Q?w3DATaZjJCi2quYPfJRXSLrLEOSRmmzOxhidjJNtg5Yga7wZBaTNMkDiXiVX?=
 =?us-ascii?Q?rZ0Zvq7lxSS2ypwPYnZmhs2dHc0/bB8aekSlbDrK5wxMD3ktA9OVsCUgqg8f?=
 =?us-ascii?Q?7F5uXIBZQErL3fYn9vkQjiE3Orfk+ZsYJAkuehUAvGuvDf6nN9ywL1+7IG3Q?=
 =?us-ascii?Q?UUg1XVb1tevw5dqxXp8TQg07gG2ihUefjdwws52DZ5bLQeOZBRlyEwslT6Bu?=
 =?us-ascii?Q?fb5Zxcz0k2Weg8075V0TaHx9GzuOv2X4Wtcx75bPoS5awlaXfX5lqH6CB9SS?=
 =?us-ascii?Q?lE+mLEqIk0qlIg49ypPxFDuFMlGWjIwAKOivfqaV5BhfxyyqvDHjPA6MwhhO?=
 =?us-ascii?Q?dokTFtp9vge2oY66yXcvuZASjJDZXtI/JpPe5Pmpk/Y2lia/4aVownxvnoF8?=
 =?us-ascii?Q?uTl714fKmbVv7maiYRP0DuFld1j06gsdqeTgNzHux/6yYLl3RvU9lAT8Dwgf?=
 =?us-ascii?Q?dTH8TjWJfZbzZ2ZzS0FAgrQPEVP7m09Pv3zzXsBUTo4z0VdqInj7qglsYZUP?=
 =?us-ascii?Q?e99MBYAi0tgtplqJ2yivG/H7Ed88gYVK7e/VMvZuwfQBsATowphPd2/zFvUg?=
 =?us-ascii?Q?uXYkmACIGBI/4E8kEvMLAPjWnL4mvRar8Wwb0q2KlXaA0NLxV5hsNGdddPYW?=
 =?us-ascii?Q?U8q2xDW4VDSeXJYL0Fxink8AfgMdFIuxqRP66PsPLIPFPNyxSHnXcg02JPYK?=
 =?us-ascii?Q?PBSD64sdOSO0ZPYAz+6e7We9pmTRBSIunVEc3b7Cc/Xr/sEywgZSmaDx+QPn?=
 =?us-ascii?Q?BYiC5seOXrP7dfVNQZONEeFxo42WxwYH3jbGKbL17IS7SIVSy02uzzGjUZQ9?=
 =?us-ascii?Q?aJ21M1t1ag04ubqSSyavtB21tsrVSiPVBLgwNRcgBhgT+uLKplcoG6e379Xe?=
 =?us-ascii?Q?+dH6ZTQvgimfe7qCZRqUhz8d3psh3L3BIAX0hoJCAQyx4tvULiJVml+4wgBN?=
 =?us-ascii?Q?swvQc6MiJUQp38vp5Wm6O1tjo/FR0j+e27w1p9a+I8tguw02gVGYC+qLBmoF?=
 =?us-ascii?Q?hqjkA7ST07KQ3givhlhjXhhEgAsxsAWCs0RDXJ+/R1DBwYiGSrToMxIjELeA?=
 =?us-ascii?Q?wgbFpkhrdBwSaWwekv8Tm6jEX+ZYLF+EzRiTduYF3A1gLyxOSoWJo0wksMda?=
 =?us-ascii?Q?liFV6YNMfYRY0/sqAH1srOzuU9w/zNDGB0ZYHYRMq5FHYJDhJ8yGGvk3x2B+?=
 =?us-ascii?Q?OiQDlKgDeYSVM5BemXDtSmksIuKMsiIJu8AKss7mkhMfhlcznUjjHxMar53A?=
 =?us-ascii?Q?RdcmkSih0OXN1bI2fmovGWcg3xhwJ/1y/47NPisgr6PicMKqDkfviVcJQi+S?=
 =?us-ascii?Q?KA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C666B8AA94C01244A6FB5F60456377AA@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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: 416721ef-ee84-4bc0-95ea-08da73f298b2
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Aug 2022 19:18:20.7154
 (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: /VoCxFfyjSdrQXs9plbw6EH3W7PzAQzIcDb9R6+Jp3MrohMbvXTUDXe1zh8l6jcKef930Q4SRmGEg5eoN8XD5w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1585
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-01_08,2022-08-01_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 phishscore=0
 malwarescore=0 suspectscore=0 spamscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2208010096
X-Proofpoint-ORIG-GUID: LW27RmJRdpqAdPdvf3bFt1DYwOOOnGrb
X-Proofpoint-GUID: LW27RmJRdpqAdPdvf3bFt1DYwOOOnGrb
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 Aug 1, 2022, at 1:18 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> lockd doesn't currently vet the start and length in nlm4 requests like
> it should, and can end up requesting locks with arguments that overflow
> to the filesystem.
>=20
> The NLM4 protocol unsigned 64-bit arguments for both start and length,
> whereas struct file_lock tracks the start and end as loff_t values. By
> the time we get around to calling nlm4svc_retrieve_args, we've lost the
> information that would allow us to determine if there was an overflow.
>=20
> Track whether the arguments will overflow as a boolean in struct
> nlm_lock, and test for that in nlm4svc_retrieve_args. While we're in
> here, get rid of the useless s64_to_loff_t helper and just directly cast
> the values.
>=20
> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D392
> Reported-by: Jan Kasiak <j.kasiak@gmail.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Thanks for tracking this down, Jeff!

I've always felt that XDR decoders should be responsible only for
parsing wire data (syntax). Architecturally it feels to me that
the conversion from protocol to local data structures should be
handled in *_retrieve_args() (semantics).

What would you think of moving @start and @len to struct nlm_lock
and handling the conversion in nlm4svc_retrieve_args() ? One
benefit of that would be to make those raw values available for
BPF and systemtap, for instance. I'm guessing it would be about
the same number of lines changed.


> ---
> fs/lockd/svc4proc.c       |  3 +++
> fs/lockd/xdr4.c           | 26 ++++++++++++--------------
> include/linux/lockd/xdr.h |  1 +
> 3 files changed, 16 insertions(+), 14 deletions(-)
>=20
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 176b468a61c7..e781ac747961 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -32,6 +32,9 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nl=
m_args *argp,
> 	if (!nlmsvc_ops)
> 		return nlm_lck_denied_nolocks;
>=20
> +	if (lock->overflow)
> +		return nlm4_fbig;
> +
> 	/* Obtain host handle */
> 	if (!(host =3D nlmsvc_lookup_host(rqstp, lock->caller, lock->len))
> 	 || (argp->monitor && nsm_monitor(host) < 0))
> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> index 856267c0864b..fd5de5481a6f 100644
> --- a/fs/lockd/xdr4.c
> +++ b/fs/lockd/xdr4.c
> @@ -20,13 +20,6 @@
>=20
> #include "svcxdr.h"
>=20
> -static inline loff_t
> -s64_to_loff_t(__s64 offset)
> -{
> -	return (loff_t)offset;
> -}
> -
> -
> static inline s64
> loff_t_to_s64(loff_t offset)
> {
> @@ -71,7 +64,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_l=
ock *lock)
> {
> 	struct file_lock *fl =3D &lock->fl;
> 	u64 len, start;
> -	s64 end;
>=20
> 	if (!svcxdr_decode_string(xdr, &lock->caller, &lock->len))
> 		return false;
> @@ -86,15 +78,21 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm=
_lock *lock)
> 	if (xdr_stream_decode_u64(xdr, &len) < 0)
> 		return false;
>=20
> +	/*
> +	 * We don't preserve the actual arguments in the call, so once
> +	 * the decoding is done, it's too late to detect overflow. Thus,
> +	 * we track it here and check for it later.
> +	 */
> +	if (start > OFFSET_MAX || (len && ((len - 1) > (OFFSET_MAX - start)))) =
{
> +		lock->overflow =3D true;
> +		return true;
> +	}
> +
> 	locks_init_lock(fl);
> 	fl->fl_flags =3D FL_POSIX;
> 	fl->fl_type  =3D F_RDLCK;
> -	end =3D start + len - 1;
> -	fl->fl_start =3D s64_to_loff_t(start);
> -	if (len =3D=3D 0 || end < 0)
> -		fl->fl_end =3D OFFSET_MAX;
> -	else
> -		fl->fl_end =3D s64_to_loff_t(end);
> +	fl->fl_start =3D (loff_t)start;
> +	fl->fl_end =3D len ? (loff_t)(start + len - 1) : OFFSET_MAX;
>=20
> 	return true;
> }
> diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
> index 398f70093cd3..a80112d18658 100644
> --- a/include/linux/lockd/xdr.h
> +++ b/include/linux/lockd/xdr.h
> @@ -39,6 +39,7 @@ struct nlm_lock {
> 	char *			caller;
> 	unsigned int		len; 	/* length of "caller" */
> 	struct nfs_fh		fh;
> +	bool			overflow; /* lock range overflow */
> 	struct xdr_netobj	oh;
> 	u32			svid;
> 	struct file_lock	fl;
> --=20
> 2.37.1
>=20

--
Chuck Lever