On Fri, Dec 3, 2021 at 11:06 AM David Howells <dhowells@xxxxxxxxxx> wrote:
>
> David Wysochanski <dwysocha@xxxxxxxxxx> 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
>
