> On Sep 7, 2021, at 7:00 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Tue, 2021-09-07 at 11:07 +0300, Dan Carpenter wrote:
>> Hello Jeff Layton,
>>
>> 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:
>>
>> net/sunrpc/addr.c:178 rpc_parse_scope_id()
>> warn: sleeping in atomic context
>>
>> net/sunrpc/addr.c
>> 161 static int rpc_parse_scope_id(struct net *net, const char *buf,
>> 162 const size_t buflen, const char *delim,
>> 163 struct sockaddr_in6 *sin6)
>> 164 {
>> 165 char *p;
>> 166 size_t len;
>> 167
>> 168 if ((buf + buflen) == delim)
>> 169 return 1;
>> 170
>> 171 if (*delim != IPV6_SCOPE_DELIMITER)
>> 172 return 0;
>> 173
>> 174 if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>> 175 return 0;
>> 176
>> 177 len = (buf + buflen) - delim - 1;
>> --> 178 p = kmemdup_nul(delim + 1, len, GFP_KERNEL);
>> 179 if (p) {
>> 180 u32 scope_id = 0;
>> 181 struct net_device *dev;
>> 182
>> 183 dev = dev_get_by_name(net, p);
>> 184 if (dev != NULL) {
>> 185 scope_id = dev->ifindex;
>> 186 dev_put(dev);
>> 187 } else {
>> 188 if (kstrtou32(p, 10, &scope_id) != 0) {
>> 189 kfree(p);
>> 190 return 0;
>> 191 }
>> 192 }
>> 193
>> 194 kfree(p);
>> 195
>> 196 sin6->sin6_scope_id = scope_id;
>> 197 return 1;
>> 198 }
>> 199
>> 200 return 0;
>> 201 }
>>
>> The call tree is:
>>
>> nfsd4_setclientid() <- disables preempt
>> -> gen_callback()
>> -> rpc_uaddr2sockaddr()
>> -> rpc_pton()
>> -> rpc_pton6()
>> -> rpc_parse_scope_id()
>>
>> The commit added a spin_lock to nfsd4_setclientid().
>>
>> 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 = 0;
>> 4028 gen_callback(new, setclid, rqstp);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> 4029 add_to_unconfirmed(new);
>> 4030 setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot;
>> 4031 setclid->se_clientid.cl_id = new->cl_clientid.cl_id;
>> 4032 memcpy(setclid->se_confirm.data, new->cl_confirm.data, sizeof(setclid->se_confirm.data));
>> 4033 new = NULL;
>> 4034 status = nfs_ok;
>> 4035 out:
>> 4036 spin_unlock(&nn->client_lock);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This is the new lock.
>>
>> 4037 if (new)
>> 4038 free_client(new);
>> 4039 if (unconf) {
>> 4040 trace_nfsd_clid_expire_unconf(&unconf->cl_clientid);
>> 4041 expire_client(unconf);
>> 4042 }
>> 4043 return status;
>>
>> regards,
>> dan carpenter
>
> (cc'ing Bruce and Chuck)
>
> Wow that is an oldie, but it does seem to be a legit bug.
>
> 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
