Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <CALF+zOkZgtfP7HrX4oP=qx2uKr3FTRHqECRqKGkRBZaz6F-jdg@mail.gmail.com>
References: <CALF+zOkZgtfP7HrX4oP=qx2uKr3FTRHqECRqKGkRBZaz6F-jdg@mail.gmail.com> <20211201085443.GA24725@kili>
To:     David Wysochanski <dwysocha@redhat.com>
Cc:     dhowells@redhat.com, Dan Carpenter <dan.carpenter@oracle.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [bug report] nfs: Convert to new fscache volume/cookie API
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <997840.1638547576.1@warthog.procyon.org.uk>
Date:   Fri, 03 Dec 2021 16:06:16 +0000
Message-ID: <997841.1638547576@warthog.procyon.org.uk>
Precedence: bulk

David Wysochanski <dwysocha@redhat.com> 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