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:     Dan Carpenter <dan.carpenter@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@fieldses.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/0e3moKXgUhSZquYZ4qAgABDIAA=
Date:   Tue, 7 Sep 2021 15:00:23 +0000
Message-ID: <622EC724-ECBF-424D-A003-46A6B8E8C215@oracle.com>
References: <20210907080732.GA20341@kili>
 <deba812574c9b898f99fc08f0c3fa23e85fc36ca.camel@kernel.org>
In-Reply-To: <deba812574c9b898f99fc08f0c3fa23e85fc36ca.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?rycg3InAt2cKA4Noy0+s8zPGOVCr70XHFUHiskzMdZXesTsZAe42/6GdRKwy?=
 =?us-ascii?Q?b6p78dzbQchDnfJVd7/N4lQHMy62LvR6B/dVSrwOgMh0URr3pkiiBzBxSAhW?=
 =?us-ascii?Q?qyYqRLCJ1eSXr5Bbhl44vyJjczT/lRFXFnz0Vf/jyKYOtSi6fnj0F0NLi15h?=
 =?us-ascii?Q?TH2/ku3gn7mWjjuraa/bD6RgDEgDfWENrKkEfM1keaTHV9tgsDNZ75ojnc0m?=
 =?us-ascii?Q?XuIarj4Rx1CSOJyM+WBz2kE1ZlXV5PutGUsc9cRXkt2yrdwR9/zYMFZeik58?=
 =?us-ascii?Q?STAQbmQEPCdbwPEAmyCCDGFm8Y5XvZKol68E8VWaCSr0kDpDU3/z67U1a9Kk?=
 =?us-ascii?Q?m5mq1Jk0ZEG0xxUlDcyRD6UgVFDqKZLuRW8pRWbbpOV+03gEunynpcEP6sCD?=
 =?us-ascii?Q?1iTyirXVUR8xuLPA0u7ceOaLMnJRxWXk7aQtjoswJmt0EO+jgLEna20kzLoy?=
 =?us-ascii?Q?YFdUmaRBlCb+IHLL0wq7D/uRbxvC+0oyQZUuiKMjHw2KSBFpriZncwBLdBNE?=
 =?us-ascii?Q?ecECDO10RgA4BjR+jN0ycDi/MtFzsg5juA58kEyvb8Wak+uXMQTATdRqVO1e?=
 =?us-ascii?Q?OMNyRr3VTxqL4+sEfwOYl9/FgueUAi/fic7P8NmpkRAE+T2AuxnvkqccKABq?=
 =?us-ascii?Q?MfyMfGbXLoVclu1BxeDnE1LyxzZ93kOSwiW0c9xAfyLvYfR3SxCdrAotH1Q1?=
 =?us-ascii?Q?QOAZS38sdDi3JVdePlUMAGHqa/KB21Hozxecxrs08VOCWn+OrQxucDT2w0Lx?=
 =?us-ascii?Q?IuwHl6KrhiQH+qlPudRPMQq0p/RwjmnZZ5Sm3VaNVcu2v81/ST0OFQKfBjlz?=
 =?us-ascii?Q?yrqr9hS6Wo4oK5AIT2dWuB/s+xLrjgYuFUY3oNTs1GC7qfEuryBiZxO+vOlB?=
 =?us-ascii?Q?RWBWeL2cMbPAszI1lGmq0+3WJtdZVZnZuQIs/I//5YMx65S+sOMtYAjPlUEX?=
 =?us-ascii?Q?rqLLDnN/9fhfBgTUtpQc0HguSMgBgJWoDcMnZHbksNbjIz+XUC2/ygWLs4uk?=
 =?us-ascii?Q?No0sw44Je7ahvWXseLe5Mga9gVd4NyYgEQ69k2U4nLXeW102RRvdH77prB6e?=
 =?us-ascii?Q?WJkf8VT4dtOoBvRCEMBgShYkRWEUbuU7D6VwywIYEhXmEsQFVyPtlvr8dca5?=
 =?us-ascii?Q?W0NyctjaRFICaLM0xNECjbyrLGBjxgeYWP8Z0sHWFTQ1Iy8h1v08KUQC8qOt?=
 =?us-ascii?Q?DaSqslJalSG312JLx9bXkTtB4o4rrzaiycS1sm2YJyKqOLrx/+KekAff+/bh?=
 =?us-ascii?Q?dibVjrJwh1WTdmnLrjWtFWMQDXs+DFtAoB4VbmBOpSoKbnwAkMQR0JO1yOrs?=
 =?us-ascii?Q?YJOG5fOItaaiztg3tYmGAUer?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <B1A3363C8128E34893D6CA382BB46E5A@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: dd631b4e-ac6e-414e-6241-08d9721037f4
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Sep 2021 15:00:23.2356
 (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: 5g/4Cbf7b8VCMv6P3DgoPzL41xbz8bPqHQJZ0VtgDaydOVNHUqeXoNLiNUqZoncob1g6IANXElktPuLyvu0gCQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4623
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10099 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 suspectscore=0
 bulkscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2108310000
 definitions=main-2109070098
X-Proofpoint-ORIG-GUID: 91HlHCZnlOFyXbbdzu4aSSSOXTXCQM2z
X-Proofpoint-GUID: 91HlHCZnlOFyXbbdzu4aSSSOXTXCQM2z
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Sep 7, 2021, at 7:00 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Tue, 2021-09-07 at 11:07 +0300, Dan Carpenter wrote:
>> Hello Jeff Layton,
>>=20
>> The patch d20c11d86d8f: "nfsd: Protect session creation and client
>> confirm using client_lock" from Jul 30, 2014, leads to the following
>> Smatch static checker warning:
>>=20
>> 	net/sunrpc/addr.c:178 rpc_parse_scope_id()
>> 	warn: sleeping in atomic context
>>=20
>> net/sunrpc/addr.c
>>    161 static int rpc_parse_scope_id(struct net *net, const char *buf,
>>    162                               const size_t buflen, const char *de=
lim,
>>    163                               struct sockaddr_in6 *sin6)
>>    164 {
>>    165         char *p;
>>    166         size_t len;
>>    167=20
>>    168         if ((buf + buflen) =3D=3D delim)
>>    169                 return 1;
>>    170=20
>>    171         if (*delim !=3D IPV6_SCOPE_DELIMITER)
>>    172                 return 0;
>>    173=20
>>    174         if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLO=
CAL))
>>    175                 return 0;
>>    176=20
>>    177         len =3D (buf + buflen) - delim - 1;
>> --> 178         p =3D kmemdup_nul(delim + 1, len, GFP_KERNEL);
>>    179         if (p) {
>>    180                 u32 scope_id =3D 0;
>>    181                 struct net_device *dev;
>>    182=20
>>    183                 dev =3D dev_get_by_name(net, p);
>>    184                 if (dev !=3D NULL) {
>>    185                         scope_id =3D dev->ifindex;
>>    186                         dev_put(dev);
>>    187                 } else {
>>    188                         if (kstrtou32(p, 10, &scope_id) !=3D 0) {
>>    189                                 kfree(p);
>>    190                                 return 0;
>>    191                         }
>>    192                 }
>>    193=20
>>    194                 kfree(p);
>>    195=20
>>    196                 sin6->sin6_scope_id =3D scope_id;
>>    197                 return 1;
>>    198         }
>>    199=20
>>    200         return 0;
>>    201 }
>>=20
>> The call tree is:
>>=20
>> nfsd4_setclientid() <- disables preempt
>> -> gen_callback()
>>   -> rpc_uaddr2sockaddr()
>>      -> rpc_pton()
>>         -> rpc_pton6()
>>            -> rpc_parse_scope_id()
>>=20
>> The commit added a spin_lock to nfsd4_setclientid().
>>=20
>> fs/nfsd/nfs4state.c
>>  4023                          trace_nfsd_clid_verf_mismatch(conf, rqstp=
,
>>  4024                                                        &clverifier=
);
>>  4025          } else
>>  4026                  trace_nfsd_clid_fresh(new);
>>  4027          new->cl_minorversion =3D 0;
>>  4028          gen_callback(new, setclid, rqstp);
>>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>=20
>>  4029          add_to_unconfirmed(new);
>>  4030          setclid->se_clientid.cl_boot =3D new->cl_clientid.cl_boot=
;
>>  4031          setclid->se_clientid.cl_id =3D new->cl_clientid.cl_id;
>>  4032          memcpy(setclid->se_confirm.data, new->cl_confirm.data, si=
zeof(setclid->se_confirm.data));
>>  4033          new =3D NULL;
>>  4034          status =3D nfs_ok;
>>  4035  out:
>>  4036          spin_unlock(&nn->client_lock);
>>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This is the new lock.
>>=20
>>  4037          if (new)
>>  4038                  free_client(new);
>>  4039          if (unconf) {
>>  4040                  trace_nfsd_clid_expire_unconf(&unconf->cl_clienti=
d);
>>  4041                  expire_client(unconf);
>>  4042          }
>>  4043          return status;
>>=20
>> regards,
>> dan carpenter
>=20
> (cc'ing Bruce and Chuck)
>=20
> Wow that is an oldie, but it does seem to be a legit bug.
>=20
> Probably we should just make rpc_parse_scope_id use an on-stack buffer
> instead of calling kmemdup_nul there. Thoughts?

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.


--
Chuck Lever