netdev - Re: [PATCH net] act_ife: Add support for machines with hard_header_len != mac

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9850b403-34b2-4a4a-7d50-8e935f3f35a2@mojatatu.com>
Date:   Thu, 22 Sep 2016 18:39:33 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Yotam Gigi <yotamg@...lanox.com>, davem@...emloft.net,
        netdev@...r.kernel.org, Roman Mashak <mrv@...atatu.com>
Subject: Re: [PATCH net] act_ife: Add support for machines with
 hard_header_len != mac_len

On 16-09-21 08:54 AM, Yotam Gigi wrote:
> Without that fix, the following could occur:
>  - On encode ingress, the total amount of skb_pushes (in lines 751 and
>    753) was more than specified in cow.
>  - On machines with hard_header_len > mac_len, the packet format was not

Just curious: What hardware would this be?


> Fixes: ef6980b6becb ("net sched: introduce IFE action")
> Signed-off-by: Yotam Gigi <yotamg@...lanox.com>
> ---
>  net/sched/act_ife.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index e87cd81..27b19ca 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -708,11 +708,13 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>  	   where ORIGDATA = original ethernet header ...
>  	 */
>  	u16 metalen = ife_get_sz(skb, ife);
> -	int hdrm = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
> -	unsigned int skboff = skb->dev->hard_header_len;
>  	u32 at = G_TC_AT(skb->tc_verd);
> -	int new_len = skb->len + hdrm;
>  	bool exceed_mtu = false;
> +	unsigned int skboff;
> +	int total_push;
> +	int reserve;
> +	int new_len;
> +	int hdrm;
>  	int err;
>
>  	if (at & AT_EGRESS) {
> @@ -724,6 +726,22 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>  	bstats_update(&ife->tcf_bstats, skb);
>  	tcf_lastuse_update(&ife->tcf_tm);
>
> +	if (at & AT_EGRESS) {
> +		/* on egress, reserve space for hard_header_len instead of
> +		 * mac_len
> +		 */
> +		skb_reset_mac_len(skb);

The skb_reset_mac_len() above is unneeded.

> +		hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;

Can you move this line outside of the if? It appears on the else
so factoring it out is useful.

> +		total_push = hdrm;
> +		reserve = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
> +	} else {
> +		/* on ingress, push mac_len as it already get parsed from tc */
> +		hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;
> +		total_push = hdrm + skb->mac_len;
> +		reserve = total_push;
> +	}
> +	new_len =  skb->len + hdrm;
> +
>  	if (!metalen) {		/* no metadata to send */
>  		/* abuse overlimits to count when we allow packet
>  		 * with no metadata
> @@ -742,19 +760,17 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>
>  	iethh = eth_hdr(skb);
>
> -	err = skb_cow_head(skb, hdrm);
> +	err = skb_cow_head(skb, reserve);
>  	if (unlikely(err)) {
>  		ife->tcf_qstats.drops++;
>  		spin_unlock(&ife->tcf_lock);
>  		return TC_ACT_SHOT;
>  	}
>
> -	if (!(at & AT_EGRESS))
> -		skb_push(skb, skb->dev->hard_header_len);
> -
> -	__skb_push(skb, hdrm);
> +	__skb_push(skb, total_push);
>  	memcpy(skb->data, iethh, skb->mac_len);
>  	skb_reset_mac_header(skb);
> +	skboff += skb->mac_len;

Above looks dangerous. Did the compiler not warn?
Maybe init skboff to skb->mac_len at the top.

Otherwise the ingress bits look good. Thanks!

Please fix above and resend with:
Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal