Re: [RFC PATCH] libcrypto/chachapoly: Use strict typing for fixed size array arguments

[Eric Biggers <ebiggers@kernel.org>]

[+Linus and Kees]

On Fri, Nov 14, 2025 at 09:33:43PM +0100, Ard Biesheuvel wrote:
> On Fri, 14 Nov 2025 at 21:23, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On Fri, Nov 14, 2025 at 07:07:07PM +0100, Ard Biesheuvel wrote:
> > > void xchacha20poly1305_encrypt(u8 *dst, const u8 *src, const size_t src_len,
> > >                                const u8 *ad, const size_t ad_len,
> > >                                const u8 (*nonce)[XCHACHA20POLY1305_NONCE_SIZE],
> > >                                const u8 (*key)[CHACHA20POLY1305_KEY_SIZE])
> > >
> > > However, this variant is checked more strictly by the compiler, and only
> > > arrays of the correct size are accepted as plain arguments (using the &
> > > operator), and so inadvertent mixing up of arguments or passing buffers
> > > of an incorrect size will trigger an error at build time.
> >
> > Interesting idea! And codegen is the same, you say?
> >
> 
> Well, the address values passed into the functions are the same.
> Whether or not some compilers may behave differently as a result is a
> different matter: I suppose some heuristics may produce different
> results knowing the fixed sizes of the inputs.
> 
> > There's another variant of this that doesn't change callsites and keeps
> > the single pointer, which more accurate reflects what the function does:
> >
> > void xchacha20poly1305_encrypt(u8 *dst, const u8 *src, const size_t src_len,
> >                                const u8 *ad, const size_t ad_len,
> >                                const u8 nonce[static XCHACHA20POLY1305_NONCE_SIZE],
> >                                const u8 key[static CHACHA20POLY1305_KEY_SIZE])
> >
> 
> Whoah!
> 
> > An obscure use of the `static` keyword, but this is what it's used for -
> > telling the compiler what size you expect the object to be. Last time I
> > investigated this, only clang respected it, but now it looks like gcc
> > does too:
> >
> >     zx2c4@thinkpad /tmp $ cat a.c
> >
> >     void blah(unsigned char herp[static 7]);
> >
> >     static void schma(void)
> >     {
> >         unsigned char good[] = { 1, 2, 3, 4, 5, 6, 7 };
> >         unsigned char bad[] = { 1, 2, 3, 4, 5, 6 };
> >         blah(good);
> >         blah(bad);
> >     }
> >     zx2c4@thinkpad /tmp $ gcc -c a.c
> >     a.c: In function ‘schma’:
> >     a.c:9:9: warning: ‘blah’ accessing 7 bytes in a region of size 6 [-Wstringop-overflow=]
> >         9 |         blah(bad);
> >           |         ^~~~~~~~~
> >     a.c:9:9: note: referencing argument 1 of type ‘unsigned char[7]’
> >     a.c:2:6: note: in a call to function ‘blah’
> >         2 | void blah(unsigned char herp[static 7]);
> >           |      ^~~~
> >     zx2c4@thinkpad /tmp $ clang -c a.c
> >     a.c:9:2: warning: array argument is too small; contains 6 elements, callee requires at least 7
> >           [-Warray-bounds]
> >         9 |         blah(bad);
> >           |         ^    ~~~
> >     a.c:2:25: note: callee declares array parameter as static here
> >         2 | void blah(unsigned char herp[static 7]);
> >           |                         ^   ~~~~~~~~~~
> >     1 warning generated.
> >
> >
> > This doesn't account for buffers that are oversize -- the less dangerous
> > case -- but maybe that's fine, to keep "normal" semantics of function
> > calls and still get some checking? And adding `static` a bunch of places
> > is easy.
> 
> Yeah if that is as portable as you say it is, it is a much better
> solution, given that the minimum size is the most important:
> inadvertently swapping two arguments will still result in a
> diagnostic, unless the buffers are the same size, in which case there
> is still a bug but not a memory safety issue. And passing a buffer
> that is too large is not a memory safety issue either.
> 
> > It could apply much wider than just chapoly.
> >
> 
> Yes, that was always the intent. I used this as an example because it
> is low hanging fruit, given that it only has a single user.
> 
> > This all makes me wish we had NT's SAL notations though...
> >
> 
> (quotation needed)

Those are some interesting ideas to make C a bit less bad!

I knew about the 'static' trick with array parameters, and I used to use
it in other projects.  It's a bit obscure, but it's in the C standard,
and both gcc and clang support the syntax.  It indeed causes clang to
start warning about too-small arrays, via -Warray-bounds which is
enabled by default.  So if we e.g. change:

    void sha256(const u8 *data, size_t len, u8 out[SHA256_DIGEST_SIZE]);

to

    void sha256(const u8 *data, size_t len, u8 out[static SHA256_DIGEST_SIZE]);

... then clang warns if a caller passes an array smaller than
SHA256_DIGEST_SIZE as 'out' (if its size is statically known).

gcc can actually warn about the too-small array regardless of 'static'.
However, gcc's warning is under -Wstringop-overflow, which we never see
because the kernel build system disables -Wstringop-overflow with gcc.

So, for now the benefit of adding 'static' would be to get warnings
about too-small arrays with one of the two supported compilers (clang).
It's too bad 'static' isn't the default behavior for array parameters in
C, but oh well...

I think it's worthwhile adding it to get better warnings, though we
should check with Linus whether he'd be okay with kernel code starting
to use this relatively obscure feature of C.

I think the 'static' trick would be better than Ard's suggestion of:

    void sha256(const u8 *data, size_t len, u8 (*out)[SHA256_DIGEST_SIZE]);

... since the "pointer to array of N elements" type would make things
difficult for callers that have any other type.  For example callers
wouldn't be able to directly use 'u8 *a', 'u8 a[M]' with size M > N, or
even a function argument 'u8 a[N]' since C implicitly converts that to
'u8 *'.  They'd need to cast it first.

- Eric