Re: [PATCH] NFSD: Avoid clashing function prototypes

[Kees Cook]

On Sat, Oct 08, 2022 at 03:49:45PM +0000, Chuck Lever III wrote:
> > On Oct 7, 2022, at 7:54 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > 
> > When built with Control Flow Integrity, function prototypes between
> > caller and function declaration must match. These mismatches are visible
> > at compile time with the new -Wcast-function-type-strict in Clang[1].
> > 
> > There were 97 warnings produced by NFS. For example:
> > 
> > fs/nfsd/nfs4xdr.c:2228:17: warning: cast from '__be32 (*)(struct nfsd4_compoundargs *, struct nfsd4_access *)' (aka 'unsigned int (*)(struct nfsd4_compoundargs *, struct nfsd4_access *)') to 'nfsd4_dec' (aka 'unsigned int (*)(struct nfsd4_compoundargs *, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> >        [OP_ACCESS]             = (nfsd4_dec)nfsd4_decode_access,
> >                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > The enc/dec callbacks were defined as passing "void *" as the second
> > argument, but were being implicitly cast to a new type. Replace the
> > argument with a variable the desired to perform the casting in the
> > function body. There are no resulting binary differences.
> 
> Hi Kees, thanks for the patch. I agree this internal API could be
> cleaner and more type-safe. A few things I noticed:
> 
> - Your patch does not apply to HEAD probably because it conflicts with
> 3fdc54646234 ("NFSD: Reduce amount of struct nfsd4_compoundargs that
> needs clearing")

Ah! Thanks, I will refresh.

> - A union type might be a better fit for this application, as that
> would avoid casting through an anonymous pointer. NFSD has a union
> type, union nfsd4_op_u, that is ideal for this.

Perfect, yes. There are similar conversions that used similar.

> Would it make sense to use "union nfsd4_op_u *", such as is done in
> fs/nfsd/current_stateid.h, in the definition of nfsd4_dec and nfsd4_enc ?

Yup; I think that'll be perfect.

> With regard to timing, I would prefer to queue this change for the
> v6.2 merge window through the nfsd tree, if that's OK with you?

Yeah, for sure. No rush. :)

-- 
Kees Cook