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
On Wed, Dec 1, 2021 at 3:55 AM Dan Carpenter <[email protected]> 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.
David Wysochanski <[email protected]> wrote:
> > 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;
It shouldn't ever overflow the array. The sprintf really shouldn't insert
more than 18 chars (comma, 16 hex digits and a NUL), but the allocated space
has a 24-char excess to handle that.
Maybe I should use:
static inline unsigned int how_many_hex_digits(unsigned int x)
{
return x ? round_up(ilog2(x) + 1, 4) / 4 : 0;
}
from fs/cachefiles/key.c to determine how much space I'm actually going to
use.
Actually, I would very much rather not include the comms parameters if I can
avoid it. They aren't really something that distinguishes volumes on servers
- they're purely a local phenomenon to distinguish local *mounts* made with
different parameters (for which nfs has different superblocks!).
David
On Fri, Dec 3, 2021 at 11:06 AM David Howells <[email protected]> wrote:
>
> David Wysochanski <[email protected]> wrote:
>
> > > 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;
>
> It shouldn't ever overflow the array. The sprintf really shouldn't insert
> more than 18 chars (comma, 16 hex digits and a NUL), but the allocated space
> has a 24-char excess to handle that.
>
> Maybe I should use:
>
> static inline unsigned int how_many_hex_digits(unsigned int x)
> {
> return x ? round_up(ilog2(x) + 1, 4) / 4 : 0;
> }
>
> from fs/cachefiles/key.c to determine how much space I'm actually going to
> use.
>
> Actually, I would very much rather not include the comms parameters if I can
> avoid it. They aren't really something that distinguishes volumes on servers
> - they're purely a local phenomenon to distinguish local *mounts* made with
> different parameters (for which nfs has different superblocks!).
>
Brainstorming...
1. Use the nfs_server.s_dev (someone can lookup the mount from
/proc/self/mountinfo)
Maybe "%u:%u", MAJOR(sb->s_dev), MINOR(sb->s_dev)
2. Use a checksum on the parameters?
> David
>
David Wysochanski <[email protected]> wrote:
> 1. Use the nfs_server.s_dev (someone can lookup the mount from
> /proc/self/mountinfo)
> Maybe "%u:%u", MAJOR(sb->s_dev), MINOR(sb->s_dev)
I think those numbers are just allocated on the fly, so they're not
consistent, so just unmounting and remounting can change them.
> 2. Use a checksum on the parameters?
I have considered hashing them. It might also make sense to only include them
if they get explicitly set.
David