netdev - Re: [PATCH bpf-next V12 4/7] bpf: add BPF-helper for MTU checking

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: <20210126101325.75097ddb@carbon>
Date:   Tue, 26 Jan 2021 10:13:25 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        colrack@...il.com, brouer@...hat.com
Subject: Re: [PATCH bpf-next V12 4/7] bpf: add BPF-helper for MTU checking

On Mon, 25 Jan 2021 23:27:22 +0100
Daniel Borkmann <daniel@...earbox.net> wrote:

> >>> +	/* At this point, skb->len exceed MTU, but as it include length of all
> >>> +	 * segments, it can still be below MTU.  The SKB can possibly get
> >>> +	 * re-segmented in transmit path (see validate_xmit_skb).  Thus, user
> >>> +	 * must choose if segs are to be MTU checked.
> >>> +	 */
> >>> +	if (skb_is_gso(skb)) {
> >>> +		ret = BPF_MTU_CHK_RET_SUCCESS;
> >>> +
> >>> +		if (flags & BPF_MTU_CHK_SEGS &&
> >>> +		    !skb_gso_validate_network_len(skb, mtu))
> >>> +			ret = BPF_MTU_CHK_RET_SEGS_TOOBIG;  
> >>
> >> I think that looks okay overall now. One thing that will easily slip through
> >> is that in the helper description you mentioned 'Check cannot use len_diff.'
> >> for BPF_MTU_CHK_SEGS flag. So right now for non-zero len_diff the user
> >> will still get BPF_MTU_CHK_RET_SUCCESS if the current length check via
> >> skb_gso_validate_network_len(skb, mtu) passes. If it cannot be checked,
> >> maybe enforce len_diff == 0 for gso skbs on BPF_MTU_CHK_SEGS?  
> > 
> > Ok. Do you want/think this can be enforced by the verifier or are you
> > simply requesting that the helper will return -EINVAL (or another errno)?  
> 
> Simple -EINVAL should be fine in this case. Generally, we can detect this from
> verifier side but I don't think the extra complexity is worth it especially given
> this is dependent on BPF_MTU_CHK_SEGS and otherwise can be non-zero.

Luckily this was also my choice in V13 that I've already send out.

https://lore.kernel.org/netdev/161159457239.321749.9067604476261493815.stgit@firesoul/
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer