linux-kernel - Re: [PATCH] NFSD: Avoid clashing function prototypes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202210081204.BE88541@keescook>
Date:   Sat, 8 Oct 2022 12:05:49 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     Jeff Layton <jlayton@...nel.org>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>,
        "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH] NFSD: Avoid clashing function prototypes

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@...omium.org> 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