Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Bruce Fields <bfields@fieldses.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [bug report] nfsd: Protect session creation and client confirm
 using client_lock
Thread-Topic: [bug report] nfsd: Protect session creation and client confirm
 using client_lock
Thread-Index: AQHXo790A7Wd0tfa/0e3moKXgUhSZquYZ4qAgABDIACAAf4ZgIAAAbgAgADWPACAAEaCAA==
Date:   Thu, 9 Sep 2021 14:31:22 +0000
Message-ID: <57B147B6-FC8F-4E70-A3E1-D449615B8355@oracle.com>
References: <20210907080732.GA20341@kili>
 <deba812574c9b898f99fc08f0c3fa23e85fc36ca.camel@kernel.org>
 <622EC724-ECBF-424D-A003-46A6B8E8C215@oracle.com>
 <20210908212605.GF23978@fieldses.org>
 <23A4CB30-F551-472F-9F2F-022C40AE1D70@oracle.com>
 <b63e52660e39cc7688921f85eadf1958ced6a869.camel@kernel.org>
In-Reply-To: <b63e52660e39cc7688921f85eadf1958ced6a869.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?cefp4kyCk5ed4QEJEFw6YU1outDDwD9XBv/OWoIZYC4M9qTawhVXef/t7Pzi?=
 =?us-ascii?Q?6IDflRUSlQaf/VKS3/nxtuvMlkMXWGoYYNmUhQafb46zw5B5lK2M82iKQsZk?=
 =?us-ascii?Q?FpwwQ2YSNZVQFpFvJQTA4bXuygCxLvu5ydC8Aj3vsBAc/KKV2Z3aJs+2nJkD?=
 =?us-ascii?Q?dLUTspAnD6jX1pGPU3PCaVMc5I02Alxf2vSNGanzOFNeVbofh4ji0RmMeIT4?=
 =?us-ascii?Q?0Jj4KepPIyI3VVzUy9j9oTKHmnMk735WSUDwX4dOlq99vFzDlH9Fg9i27ILr?=
 =?us-ascii?Q?kQKcZM2ezNl+J23swxMb89DgiJJrbNoQEKRPV7m7IXAkay3hGzQaCqLEIx/V?=
 =?us-ascii?Q?yWPExkjpOv4T7pzWMRnxXUb15dzc41jqK3z4w9t9O1XurEoXzVd7jffJvHHB?=
 =?us-ascii?Q?btFy+AfNeWdz/sbmRqNfcycBPMywbedrp9auF+SWFR65Q76jnwDCTdr1wHjy?=
 =?us-ascii?Q?+1GHCD109ZyUHMAjczf8XXbYDSgqNMoUK+3PqD869jYMTpgPZ6pICDV5+DJN?=
 =?us-ascii?Q?wIxmNp3sNpu/uzNR/MYw24wN8nouEMXv1JU/eOIu88ZYM2onnVdjawq8KHX/?=
 =?us-ascii?Q?ZcJeLTmASZpor3q9MKCAmLJQvCip9c1OdVQQkugsee/Li/cCJcMeVeVjIvLi?=
 =?us-ascii?Q?2OLYModyKNWh3tm1zp8qsOC0358lILO4C05sWFVEi78HuJsk+4AG9f3rDl6q?=
 =?us-ascii?Q?5Ty5BREMGjSqIFjk+1pp5PK3iymS12URZ+7Zi6sIVRS9GSxQvytwTbVSZSqs?=
 =?us-ascii?Q?aApAPzYrKN6uTEpPGLgUa0YqLYM/Rldu2ksNlZcl91qZNu0WbqUENP6nHj2q?=
 =?us-ascii?Q?PiOIyfLHiC3c++ismpbLnWpjQi76sNQrdov8RV/HLVSGNInEGW3Q17a2BScH?=
 =?us-ascii?Q?87hN24HcOu+1JhFEzWLP4+vyq0VfaCAgNwS+dqB6PYCadcI5Sw15fr8c33hE?=
 =?us-ascii?Q?j49gWYma94YgvhbkGTPLtxEIfSC5KQ5yBfLgV6V/gBhe6zR0trb6ithl/RrP?=
 =?us-ascii?Q?yyWntzrfhzkc8kP4hQm0NtKaJLqWO9Plf+QGvaGORH1k3oW0kIeFcOFhapKl?=
 =?us-ascii?Q?YrGOwjGNUAjwH3DsJL5gn9sWDPdkFuY3Y2R/8xTNG4iDod0/8yuRda4E3li+?=
 =?us-ascii?Q?oIlbebQioo+zBbI32TlUS3KJFOSJCwvnA7k0lnMSniQFGblVcqD8RQjf0p8D?=
 =?us-ascii?Q?Frk6XgkTWLmj9ccv+QXiEUkL4i75OFUXla+FgF7PhcX94osWQi8MbXME/mxb?=
 =?us-ascii?Q?bboME6TAtcNxbRqBzGHqTEwQINy8GoJieqgo6BTBoUAPkrme/P6S/E6qLyMm?=
 =?us-ascii?Q?4ezIaK/cAn7HEWycgF7ukMme?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <719D894AF8E2AD43BACDB8F2D7E18598@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: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b662fa1b-ba15-4842-684a-08d9739e7f50
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Sep 2021 14:31:22.5687
 (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: ions29ERzjKY4mR59OT3TOsUKJRX8BEMDQgeleXf9o2DxhsrMFtPm3q1XAl2GfCvv2MDHT7xgXhb4uVkQX7SXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3828
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10101 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 adultscore=0
 malwarescore=0 mlxscore=0 spamscore=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109090089
X-Proofpoint-GUID: W1dqTWXKAj0CHzBEpFP2Sb9GWW6PEMDT
X-Proofpoint-ORIG-GUID: W1dqTWXKAj0CHzBEpFP2Sb9GWW6PEMDT
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Sep 9, 2021, at 6:19 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Wed, 2021-09-08 at 21:32 +0000, Chuck Lever III wrote:
>>=20
>>> On Sep 8, 2021, at 5:26 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>>=20
>>> On Tue, Sep 07, 2021 at 03:00:23PM +0000, Chuck Lever III wrote:
>>>> We have IPV6_SCOPE_ID_LEN as a maximum size of the scope ID,
>>>> and it's not a big value. As long as boundary checking is made
>>>> to be sufficient, then a stack residency for the device name
>>>> should be safe.
>>>=20
>>> Something like this?  (Or are you making a patch?
>>=20
>> I thought Jeff was going to handle it? More below.
>>=20
>=20
> No, sorry... I was just suggesting a potential fix. I'd probably rather
> you guys fix it since you're better positioned to test this at the
> moment.
>=20
>>=20
>>> I'm not even sure how to test.)
>>> are
>>> --b.
>>>=20
>>> diff --git a/net/sunrpc/addr.c b/net/sunrpc/addr.c
>>> index 6e4dbd577a39..d435bffc6199 100644
>>> --- a/net/sunrpc/addr.c
>>> +++ b/net/sunrpc/addr.c
>>> @@ -162,8 +162,10 @@ static int rpc_parse_scope_id(struct net *net, con=
st char *buf,
>>> 			      const size_t buflen, const char *delim,
>>> 			      struct sockaddr_in6 *sin6)
>>> {
>>> -	char *p;
>>> +	char p[IPV6_SCOPE_ID_LEN + 1];
>>> 	size_t len;
>>> +	u32 scope_id =3D 0;
>>> +	struct net_device *dev;
>>>=20
>>> 	if ((buf + buflen) =3D=3D delim)
>>> 		return 1;
>>> @@ -175,29 +177,23 @@ static int rpc_parse_scope_id(struct net *net, co=
nst char *buf,
>>> 		return 0;
>>>=20
>>> 	len =3D (buf + buflen) - delim - 1;
>>> -	p =3D kmemdup_nul(delim + 1, len, GFP_KERNEL);
>>> -	if (p) {
>>> -		u32 scope_id =3D 0;
>>> -		struct net_device *dev;
>>> -
>>> -		dev =3D dev_get_by_name(net, p);
>>> -		if (dev !=3D NULL) {
>>> -			scope_id =3D dev->ifindex;
>>> -			dev_put(dev);
>>> -		} else {
>>> -			if (kstrtou32(p, 10, &scope_id) !=3D 0) {
>>> -				kfree(p);
>>> -				return 0;
>>> -			}
>>> -		}
>>> -
>>> -		kfree(p);
>>> -
>>> -		sin6->sin6_scope_id =3D scope_id;
>>> -		return 1;
>>> +	if (len > IPV6_SCOPE_ID_LEN)
>>> +		return 0;
>>> +
>>> +	memcpy(p, delim + 1, len);
>>> +	p[len] =3D 0;
>>=20
>> If I recall correctly, Linus prefers us to use the str*()
>> functions instead of raw memcpy() in cases like this.
>=20
> I hadn't heard that.

I'm paraphrasing these:

https://lore.kernel.org/lkml/CAHk-=3Dwj5Pp5J-CAPck22RSQ13k3cEOVnJHUA-WocAZq=
CJK1BZw@mail.gmail.com/

https://lore.kernel.org/lkml/CAHk-=3DwjWosrcv2=3D6m-=3DYgXRKev=3D5cnCg-1Ehq=
DpbRXT5z6eQmg@mail.gmail.com/


> If you already know the length to be copied, then
> strcpy and the like tend to be less efficient since they continually
> have to check for null terminators as they walk the source string.

I'm sure there's one str helper that comes close to what we need.
Here efficiency doesn't really matter, and the size of the device
string is always going to be in the single digits.

The priority is correctness.


>>> +
>>> +	dev =3D dev_get_by_name(net, p);
>>> +	if (dev !=3D NULL) {
>>> +		scope_id =3D dev->ifindex;
>>> +		dev_put(dev);
>>> +	} else {
>>> +		if (kstrtou32(p, 10, &scope_id) !=3D 0)
>>> +			return 0;
>>> 	}
>>>=20
>>> -	return 0;
>>> +	sin6->sin6_scope_id =3D scope_id;
>>> +	return 1;
>>> }
>>>=20
>>> static size_t rpc_pton6(struct net *net, const char *buf, const size_t =
buflen,
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever