MIME-Version: 1.0
References: <20211201085443.GA24725@kili>
In-Reply-To: <20211201085443.GA24725@kili>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Fri, 3 Dec 2021 08:39:30 -0500
Message-ID: <CALF+zOkZgtfP7HrX4oP=qx2uKr3FTRHqECRqKGkRBZaz6F-jdg@mail.gmail.com>
Subject: Re: [bug report] nfs: Convert to new fscache volume/cookie API
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        David Howells <dhowells@redhat.com>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 1, 2021 at 3:55 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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
>

Thanks for the report.

The nfs_append_int() was actually a portion that David Howells
wrote in the latest round of fscache API conversion.
So I'm not sure if he wants to fix this up or if I should give it a go.