[bug report] nfs: Convert to new fscache volume/cookie API

* [bug report] nfs: Convert to new fscache volume/cookie API
@ 2021-12-01  8:54 Dan Carpenter
  2021-12-03 13:39 ` David Wysochanski
  2021-12-03 16:06 ` David Howells
  0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-12-01  8:54 UTC (permalink / raw)
  To: dwysocha; +Cc: linux-nfs

Hi Dave,

The patch 1234f5681081: "nfs: Convert to new fscache volume/cookie
API" from Nov 14, 2020, leads to the following Smatch static checker
warning:

	fs/nfs/fscache.c:32 nfs_append_int()
	warn: array off by one? 'key[(*_len)++]'

This is an unpublished check which assumes all > comparisons are wrong
until proven otherwise.

fs/nfs/fscache.c
    27 static bool nfs_append_int(char *key, int *_len, unsigned long long x)
    28 {
    29         if (*_len > NFS_MAX_KEY_LEN)
    30                 return false;
    31         if (x == 0)
--> 32                 key[(*_len)++] = ',';
    33         else
    34                 *_len += sprintf(key + *_len, ",%llx", x);
    35         return true;
    36 }

This function is very badly broken.  As the checker suggests, the >
should be >= to prevent an array overflow.  But it's actually off by
two because we have to leave space for the NUL terminator so the buffer
is full when "*_len == NFS_MAX_KEY_LEN - 1".  That means the check
should be:

	if (*_len >= NFS_MAX_KEY_LEN - 1)
		return false;

Except NFS_MAX_KEY_LEN is not the correct limit.  The buffer is larger
than that.

Unfortunately if we fixed the array overflow check then the sprintf()
will now corrupt memory...  There is a missing check after the sprintf()
so it does not tell you if we printed everything or not.  It just
returns true and the next caller detects the overflow.

I feel like append() functions are easier to use if we keep the start
pointer and and len value constant and just modify the *offset.

static bool nfs_append_int(char *key, int *off, int len, unsigned long long x)
{
	if (*off >= len - 1)
		return false;

	if (x == 0)
		key[(*off)++] = ',';
	else
		*off + snprintf(key + *off, len - *off, ",%llx", x);

	if (*off >= len)
		return false;
	return true;
}

[ snip ]

    86  void nfs_fscache_get_super_cookie(struct super_block *sb, const char *uniq, int ulen)
    87  {
    88          struct nfs_server *nfss = NFS_SB(sb);
    89          unsigned int len = 3;
    90          char *key;
    91  
    92          if (uniq) {
    93                  nfss->fscache_uniq = kmemdup_nul(uniq, ulen, GFP_KERNEL);
    94                  if (!nfss->fscache_uniq)
    95                          return;
    96          }
    97  
    98          key = kmalloc(NFS_MAX_KEY_LEN + 24, GFP_KERNEL);
    99          if (!key)
   100                  return;
   101  
   102          memcpy(key, "nfs", 3);
   103          if (!nfs_fscache_get_client_key(nfss->nfs_client, key, &len) ||
   104              !nfs_append_int(key, &len, nfss->fsid.major) ||

So then we do:

	len = NFS_MAX_KEY_LEN + 24;

It's not totally clear to me where the 24 comes from or I would have
sent a patch instead of a bug report.

	memcpy(key, "nfs", 3);
	off = 3;

	if (!nfs_fscache_get_client_key(nfss->nfs_client, key, &off) ||
	    !nfs_append_int(key, &off, len, nfss->fsid.major) ||
	    !nfs_append_int(key, &off, len, nfss->fsid.minor) ||

   105              !nfs_append_int(key, &len, nfss->fsid.minor) ||
   106              !nfs_append_int(key, &len, sb->s_flags & NFS_SB_MASK) ||
   107              !nfs_append_int(key, &len, nfss->flags) ||
   108              !nfs_append_int(key, &len, nfss->rsize) ||
   109              !nfs_append_int(key, &len, nfss->wsize) ||
   110              !nfs_append_int(key, &len, nfss->acregmin) ||
   111              !nfs_append_int(key, &len, nfss->acregmax) ||
   112              !nfs_append_int(key, &len, nfss->acdirmin) ||
   113              !nfs_append_int(key, &len, nfss->acdirmax) ||
   114              !nfs_append_int(key, &len, nfss->client->cl_auth->au_flavor))
   115                  goto out;
   116  
   117          if (ulen > 0) {
   118                  if (ulen > NFS_MAX_KEY_LEN - len)

This check is also off.  It does not take into consideration the comma
or the NUL terminator.  We need a "+ 2".

			if (ulen + 2 > len - off) {

   119                          goto out;
   120                  key[len++] = ',';
   121                  memcpy(key + len, uniq, ulen);
   122                  len += ulen;
   123          }
   124          key[len] = 0;
   125  
   126          /* create a cache index for looking up filehandles */

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 5+ messages in thread