Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 18:42 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 27, 2023 at 02:23:27PM +0200, Thomas Haller wrote:
> > > We almost can compile everything with "-Wstrict-overflow" (which
> > > depends
> > > on the optimization level). In a quest to make that happen, rework
> > > nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned:
> > > 
> > >     $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c
> > > -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3
> > >     src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’:
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >       356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx,
> > > int del)
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     cc1: all warnings being treated as errors
> > > 
> > > The previous code was needlessly confusing. Keeping track of an
> > > index
> > > variable "i" and a "ptr" was redundant. The signed "i" variable
> > > caused a
> > > "-Wstrict-overflow" warning, but it can be dropped completely.
> > > 
> > > While at it, there is also almost no need to ever truncate the bits
> > > that
> > > we parse. Only the callers of the new skip_delim_trunc() required
> > > the
> > > truncation.
> > > 
> > > Also, introduce new skip_delim() and skip_delim_trunc() methods,
> > > which
> > > point right *after* the delimiter to the next word.  Contrary to
> > > nf_osf_strchr(), which leaves the pointer at the end of the
> > > previous
> > > part.
> > > 
> > > Also, the parsing code using strchr() requires that the overall
> > > buffer
> > > (obuf[olen]) is NUL terminated. And the caller in fact ensured that
> > > too.
> > > There is no point in having a "olen" parameter, we require the
> > > string to
> > > be NUL terminated (which already was implicitly required).  Drop
> > > the
> > > "olen" parameter. On the other hand, it's unclear what ensures that
> > > we
> > > don't overflow the "opt" output buffer. Pass a "optlen" parameter
> > > and
> > > ensure we don't overflow the buffer.
> > 
> > Nice.
> > 
> > IIRC, this code was copied and pasted from iptables. Maybe porting
> > this patch there would be also good.
> 
> I will do that, after the patch was merged (and the final version
> known).
> 
> > BTW, did you test this patch with the pf.os file that nftables ships
> > in?
> 
> Right. I need to point out, that I did not test this. So it might be
> horribly broken. My Fedora kernel builds without CONFIG_NFT_OSF, so the
> shell tests are skipped.
> 
> How can pf.os used?

According to code, pf.os file with signatures needs to be placed here:

#define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/nftables/osf/pf.os"

then, you can start matching on OS type, see 'osf' expression in
manpage. Note there is a "unknown" OS type when it does not guess the OS.