From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Boyang Xue <bxue@redhat.com>
Subject: Re: [PATCH] nfsd: don't hand out delegation on setuid files being
 opened for write
Thread-Topic: [PATCH] nfsd: don't hand out delegation on setuid files being
 opened for write
Thread-Index: AQHZMkg9QowbIAXTEkyy+4RjwAMvNa6yYYqA
Date:   Fri, 27 Jan 2023 15:21:40 +0000
Message-ID: <D439961A-3E64-425F-8385-E5179325576C@oracle.com>
References: <20230127120933.7056-1-jlayton@kernel.org>
In-Reply-To: <20230127120933.7056-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?8UthYbptQgnuTTorHc7GDqdBf+6MhMGjR/4p3F6hfvurDtIwl2nb5Z2p7nM/?=
 =?us-ascii?Q?X+2a902qaDcJLnMqe6Ke6ReVgtJ7l+OL2APlki9Z8s4C1/a8at4m4RBo0WFB?=
 =?us-ascii?Q?aWhPNKDzdOudYsHcU/xKUZi9BsHQiuFYKszErFwtrnaAAs7YBn2+47t2sGdJ?=
 =?us-ascii?Q?WUBsrITrsJkO8J39viEl1zUcCzmfdDrB9osoycGpeppkmm1UKvO6UQ5gtmMW?=
 =?us-ascii?Q?hF2xL/olR7oD7QYH3uvq/79RW9NP/RJF1ukbZeDosCy5qZ9+waSf16ZomU4S?=
 =?us-ascii?Q?DzI/M+PUjxRgCHtj3HBB/7wSJ03ZGRfM4/lNwzY4oD3MZA1J6V3n8b+PE5kI?=
 =?us-ascii?Q?KVvYfG7ZH7/fuTKgy9SW33n00hO0d4Et0UDi6+EOJYUWkZgAm8efieXOyM/3?=
 =?us-ascii?Q?tufySeUi7g4XKZWkTN9kCMFkG/4OyxTbcZuWY7tc+ycVbpm4GBJ5MYQLRGc3?=
 =?us-ascii?Q?XnlGimEPuAXtrUj14uMuBuv1ShvRF6k/DvtqtnC6NongDz33xp5Q8uPfLUc7?=
 =?us-ascii?Q?Zgyd0NJ1XUPPK4x1MrRRtxbWEJ3j9vuY2iMgrybr643VIEnm1wViYHHsomEy?=
 =?us-ascii?Q?iG9s4WkFs6huAx2/tDAp5kndbCVk81UleoH8uGgLvIMXWcJSnMTrc/v/dWVI?=
 =?us-ascii?Q?O1y3k1A9Olns6Qzt7b0MzF1EsipZLIY8qMhTwuOGayPG7V5v0quVdcFEDs5Y?=
 =?us-ascii?Q?MeJ36+u7SC1Nm0CthsXusN8I+SQDQKKGeJr2HKKS18lMAAFhKaEskCNYd5Tk?=
 =?us-ascii?Q?5xO9F19t2hnaK1gmTmUVSI8KpXtYZGT9aaTUqONI8PleneaddDB2hCxPEqbV?=
 =?us-ascii?Q?uvOkBAOBC1iIo0zKfeVGDst9mL10TRoIWFMLpl/4ODIGbZ9N+mKaXeQxFgJk?=
 =?us-ascii?Q?u7QoOTHg4Wofzv8jP74UK2hz+PS/3aBK3YO4daBA5M+HUhh8JyYzIyyRXCV7?=
 =?us-ascii?Q?SQ+0VVxEZBeCF7Rm8hdTXKHprgb90OG73JzUzFozXu74sgOK6kX4FRthyVrn?=
 =?us-ascii?Q?kKCyBF2wsJXP+sPwk8tr69afcf3OVsMM2+BvDzJ9fkN03n8BuK0KOt2IadUk?=
 =?us-ascii?Q?NcOaATPtcNoQYh2Kbigc2ZPZ6GnDgMhzC1MDqVbV4tS8tz0spukgs4hiTA6g?=
 =?us-ascii?Q?lrQ2lgQCyMWm1Rhvh4S4ucVf3QE8unXLMt+lzjNLEwiQeHyUAQQyoXFaLgQy?=
 =?us-ascii?Q?gSTzQSPL+znpRjXvaMUrGR4e3apE8Kh4czDdvuW1JMRVd9HVTx5kZXL8k5l1?=
 =?us-ascii?Q?LRNd1cDxx7vBNCdrpaWNvJk92arlVuXk26cxU9eKsNiQ2wug6O7HEUAFtkLR?=
 =?us-ascii?Q?jBdPl3KdSd7EK2YJ8BjAiRS52hXapuMCjNu21s0wscMA24xHnHFpqEOMBDPu?=
 =?us-ascii?Q?yk7emFwWF6J3bP3V2QA3Md+pvquUhcKU/zEgp2ef1WMMcynTqsDhXqBKdglI?=
 =?us-ascii?Q?NZj3uzjnIZ4vvivJwaK88kXhc4fudSDDfgpQl6BNMa8p4is4OkBGl0iLhuY3?=
 =?us-ascii?Q?pYS9s2Hm71qatLjDvT1v1eJV5pjcRiYjzOH6AUOtjGI2cI0JYqu+ffL/js+j?=
 =?us-ascii?Q?jFsHIVFDXmAIC9zE0PewXANqIma8lqd6+B+x1t1VueH1mWHROnfoTVv9GvPj?=
 =?us-ascii?Q?9A=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2924146398FCCC48AEA31F7F0D88EDA3@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: zd7fCPWGwwraloUfmU/iAwFEdFqnzjCoZxVqYvzm9OVU/bW0OZ3oJVlrHEPmPs1ZQKz47iMzbWE5daQuvVR3CfMEqSBWhMQC5ctsk1oW2pRnczsBu0Dv9Az0lEms5sTUqFGZc0+6QveaM7GXGt9qlk1g8VBcLCtcg5LhR0whjtXShoQroc3HkF6/vmttQyii1Gk69BEm5s1ZAFYmoev0ZS8A3xH0LNYejyOdgN9bR8ld4QNfjTl9I82grBxoa/DIfBu9Mm4fQPvcA2UF0Fn8j3KSULFrPqOJOEOW8RjKQpV+KK0n2F/e15ff5qotNQOMiA2eJjiz9qjl4z5vZclO95zTLlpVKS3cEXr4+JKw/57CP9q6DKWBwQpuUpgYSv4Ez7O4nOvPWQaItEij6BuHyYsnm2l4DwsWbA9NkVRxSdlFz/CxG/3mMyMBLuSSth5IF27l7l18VNBQVQGGXU3bJX8tMiFDDcnhUEbyxUuKAjsyZyiyeoglqC95Rsrdq3Hy1uGThVc1T+YVRML2n3ERtPqSrgAS2LfsK6hno78aGkDuaGQnEV38Ya46fNOs/ssRCkUfISuvy3P1ayAIeszyR08X13idCLpjUlJq8kP3narZhPe23CuliaZMOyxzcScBP7ebN3D0M+1e6zSmWfQyEnM0rl4ApiXSZseIbXMEJ8vAch3h0UVCWuryI6P2BqDhBHwdnw2ZzlSM/YYq3bGRLxviKI53jcg1y7OCdHm2v4SyvL+tXnDTPBRQJYZzO3qBmKMgvJ7VHP4VSMSBVNinWvypd6H1OZ5Za/Xsytjgf2v+KM3JMWqNJ98A439kSxhU
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: 599480ed-af9e-43ea-a880-08db007a30bf
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jan 2023 15:21:40.6777
 (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: 5RkmfFZ+pDp/Ct9++UQAFbVICc5gAhdFyrIxlRigvW0monsyxNC6E3KaKih/3pmV/miCD9DrSkSy6sp+CRN+pQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5596
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-27_09,2023-01-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 spamscore=0
 bulkscore=0 mlxlogscore=887 malwarescore=0 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2301270143
X-Proofpoint-ORIG-GUID: Wp6vB7xIeg5qk_ABo2XQf6Z26YncAMYN
X-Proofpoint-GUID: Wp6vB7xIeg5qk_ABo2XQf6Z26YncAMYN
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jan 27, 2023, at 7:09 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> We had a bug report that xfstest generic/355 was failing on NFSv4.0.
> This test sets various combinations of setuid/setgid modes and tests
> whether DIO writes will cause them to be stripped.
>=20
> What I found was that the server did properly strip those bits, but
> the client didn't notice because it held a delegation that was not
> recalled. The recall didn't occur because the client itself was the
> one generating the activity and we avoid recalls in that case.
>=20
> Clearing setuid bits is an "implicit" activity. The client didn't
> specifically request that we do that, so we need the server to issue a
> CB_RECALL, or avoid the situation entirely by not issuing a delegation.
>=20
> The easiest fix here is to simply not give out a delegation if the file
> is being opened for write, and the mode has the setuid and/or setgid bit
> set. Note that there is a potential race between the mode and lease
> being set, so we test for this condition both before and after setting
> the lease.
>=20
> This patch fixes generic/355, generic/683 and generic/684 for me.

generic/355 2s ...  1s

That's good.

generic/683 2s ... [not run] xfs_io falloc  failed (old kernel/wrong fs?)
generic/684 2s ... [not run] xfs_io fpunch  failed (old kernel/wrong fs?)

What am I doing wrong?


> Reported-by: Boyang Xue <bxue@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/nfs4state.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e61b878a4b45..ace02fd0d590 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5421,6 +5421,23 @@ nfsd4_verify_deleg_dentry(struct nfsd4_open *open,=
 struct nfs4_file *fp,
> 	return 0;
> }
>=20
> +/*
> + * We avoid breaking delegations held by a client due to its own activit=
y, but
> + * clearing setuid/setgid bits on a write is an implicit activity and th=
e client
> + * may not notice and continue using the old mode. Avoid giving out a de=
legation
> + * on setuid/setgid files when the client is requesting an open for writ=
e.
> + */
> +static int
> +nfsd4_verify_setuid_write(struct nfsd4_open *open, struct nfsd_file *nf)
> +{
> +	struct inode *inode =3D file_inode(nf->nf_file);
> +
> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_WRITE) &&
> +	    (inode->i_mode & (S_ISUID|S_ISGID)))
> +		return -EAGAIN;
> +	return 0;
> +}
> +
> static struct nfs4_delegation *
> nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> 		    struct svc_fh *parent)
> @@ -5454,6 +5471,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct=
 nfs4_ol_stateid *stp,
> 	spin_lock(&fp->fi_lock);
> 	if (nfs4_delegation_exists(clp, fp))
> 		status =3D -EAGAIN;
> +	else if (nfsd4_verify_setuid_write(open, nf))
> +		status =3D -EAGAIN;
> 	else if (!fp->fi_deleg_file) {
> 		fp->fi_deleg_file =3D nf;
> 		/* increment early to prevent fi_deleg_file from being
> @@ -5494,6 +5513,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struc=
t nfs4_ol_stateid *stp,
> 	if (status)
> 		goto out_unlock;
>=20
> +	/*
> +	 * Now that the deleg is set, check again to ensure that nothing
> +	 * raced in and changed the mode while we weren't lookng.
> +	 */
> +	status =3D nfsd4_verify_setuid_write(open, fp->fi_deleg_file);
> +	if (status)
> +		goto out_unlock;
> +
> 	spin_lock(&state_lock);
> 	spin_lock(&fp->fi_lock);
> 	if (fp->fi_had_conflict)
> --=20
> 2.39.1
>=20

--
Chuck Lever