On 2023/8/3 05:40, Nick Desaulniers wrote:
On Wed, Aug 2, 2023 at 3:25 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
I noticed that the function in question already has a guard:
322 if (hostname && memchr(hostname, '/', hostname_len) != NULL) {
Which implies that hostname COULD be NULL.
Should this perhaps simply be rewritten as:
if (!hostname)
return NULL;
if (memchr(...) != NULL)
...
Rather than bury yet another guard for the same check further down in
the function? Check once and bail early.
Hi, Nick Desaulnier,
This may disrupt the logic of this function. So maybe the past one is better.
322 if (!hostname)
323 return NULL;
^^^^^^^^^^^^
If we return in this place.
324 if (memchr(hostname, '/', hostname_len) != NULL) {
325 if (printk_ratelimit()) {
326 printk(KERN_WARNING "Invalid hostname \"%.*s\" "
327 "in NFS lock request\n",
328 (int)hostname_len, hostname);
329 }
330 return NULL;
331 }
332
333 retry:
334 spin_lock(&nsm_lock);
335
336 if (nsm_use_hostnames && hostname != NULL)
337 cached = nsm_lookup_hostname(&ln->nsm_handles,
338 hostname, hostname_len);
339 else
340 cached = nsm_lookup_addr(&ln->nsm_handles, sap);
^^^^^^^^^^^^^^^
This case will be broken when hostname is NULL.
341
342 if (cached != NULL) {
343 refcount_inc(&cached->sm_count);
344 spin_unlock(&nsm_lock);
345 kfree(new);
346 dprintk("lockd: found nsm_handle for %s (%s), "
347 "cnt %d\n", cached->sm_name,
348 cached->sm_addrbuf,
349 refcount_read(&cached->sm_count));
350 return cached;
351 }
I noticed a related bug which Smatch doesn't find, because of how Smatch
handles the dprintk macro.
Hi, Dan,
Great eye!
Should I send a separate patch for this bug and add you as Reported-by ?
fs/lockd/host.c
truct nlm_host *nlmclnt_lookup_host(const struct sockaddr *sap,
217 const size_t salen,
218 const unsigned short protocol,
219 const u32 version,
220 const char *hostname,
221 int noresvport,
222 struct net *net,
223 const struct cred *cred)
224 {
225 struct nlm_lookup_host_info ni = {
226 .server = 0,
227 .sap = sap,
228 .salen = salen,
229 .protocol = protocol,
230 .version = version,
231 .hostname = hostname,
232 .hostname_len = strlen(hostname),
^^^^^^^^
Dereferenced
233 .noresvport = noresvport,
234 .net = net,
235 .cred = cred,
236 };
237 struct hlist_head *chain;
238 struct nlm_host *host;
239 struct nsm_handle *nsm = NULL;
240 struct lockd_net *ln = net_generic(net, lockd_net_id);
241
242 dprintk("lockd: %s(host='%s', vers=%u, proto=%s)\n", __func__,
243 (hostname ? hostname : "<none>"), version,
^^^^^^^^
Checked too late.
244 (protocol == IPPROTO_UDP ? "udp" : "tcp"));
245
regards,
dan carpenter
