netdev - Re: [PATCH] net: hsr: Add support for redbox supervision frames

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: <20211013093753.11c7cd91@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 13 Oct 2021 09:37:53 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Andreas Oetken <ennoerlangen@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Murali Karicheri <m-karicheri2@...com>
Subject: Re: [PATCH] net: hsr: Add support for redbox supervision frames

On Wed, 13 Oct 2021 18:13:48 +0200 Andreas Oetken wrote:
> Am Mittwoch, dem 13.10.2021 um 08:50 -0700 schrieb Jakub Kicinski:
> > On Wed, 13 Oct 2021 09:29:51 +0200 Andreas Oetken wrote:  
> > > added support for the redbox supervision frames
> > > as defined in the IEC-62439-3:2018.
> > > 
> > > Signed-off-by: Andreas Oetken <andreas.oetken@...mens-energy.com>  
> > 
> > This does not apply to netdev/net-next.  
> Sorry, I will not include it next time.

To be clear - please rebase on that tree, and also include 
[PATCH net-next] in the subject.

> > >  
> > > -       if (hsr_sup_tag->HSR_TLV_type != HSR_TLV_ANNOUNCE &&
> > > -           hsr_sup_tag->HSR_TLV_type != HSR_TLV_LIFE_CHECK &&
> > > -           hsr_sup_tag->HSR_TLV_type != PRP_TLV_LIFE_CHECK_DD &&
> > > -           hsr_sup_tag->HSR_TLV_type != PRP_TLV_LIFE_CHECK_DA)
> > > +       if (hsr_sup_tag->tlv.HSR_TLV_type != HSR_TLV_ANNOUNCE &&
> > > +           hsr_sup_tag->tlv.HSR_TLV_type != HSR_TLV_LIFE_CHECK &&
> > > +           hsr_sup_tag->tlv.HSR_TLV_type != PRP_TLV_LIFE_CHECK_DD
> > > &&
> > > +           hsr_sup_tag->tlv.HSR_TLV_type != PRP_TLV_LIFE_CHECK_DA)
> > >                 return false;
> > > -       if (hsr_sup_tag->HSR_TLV_length != 12 &&
> > > -           hsr_sup_tag->HSR_TLV_length != sizeof(struct
> > > hsr_sup_payload))
> > > +       if (hsr_sup_tag->tlv.HSR_TLV_length != 12 &&
> > > +           hsr_sup_tag->tlv.HSR_TLV_length != sizeof(struct
> > > hsr_sup_payload))
> > >                 return false;
> > >  
> > >         return true;
> > > diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> > > index bb1351c38397..e7c6efbc41af 100644
> > > --- a/net/hsr/hsr_framereg.c
> > > +++ b/net/hsr/hsr_framereg.c
> > > @@ -265,6 +265,7 @@ void hsr_handle_sup_frame(struct hsr_frame_info
> > > *frame)
> > >         struct hsr_port *port_rcv = frame->port_rcv;
> > >         struct hsr_priv *hsr = port_rcv->hsr;
> > >         struct hsr_sup_payload *hsr_sp;
> > > +       struct hsr_sup_tlv *hsr_sup_tlv;
> > >         struct hsr_node *node_real;
> > >         struct sk_buff *skb = NULL;
> > >         struct list_head *node_db;
> > > @@ -312,6 +313,40 @@ void hsr_handle_sup_frame(struct
> > > hsr_frame_info *frame)
> > >                 /* Node has already been merged */
> > >                 goto done;
> > >  
> > > +       /* Leave the first HSR sup payload. */
> > > +       skb_pull(skb, sizeof(struct hsr_sup_payload));
> > > +
> > > +       /* Get second supervision tlv */
> > > +       hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
> > > +       /* And check if it is a redbox mac TLV */
> > > +       if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
> > > +               /* We could stop here after pushing
> > > hsr_sup_payload,
> > > +                * or proceed and allow macaddress_B and for
> > > redboxes.
> > > +                */
> > > +               /* Sanity check length */
> > > +               if (hsr_sup_tlv->HSR_TLV_length != 6) {
> > > +                       skb_push(skb, sizeof(struct
> > > hsr_sup_payload));
> > > +                       goto done;
> > > +               }
> > > +               /* Leave the second HSR sup tlv. */
> > > +               skb_pull(skb, sizeof(struct hsr_sup_tlv));
> > > +
> > > +               /* Get redbox mac address. */
> > > +               hsr_sp = (struct hsr_sup_payload *)skb->data;
> > > +
> > > +               /* Check if redbox mac and node mac are equal. */
> > > +               if (!ether_addr_equal(node_real->macaddress_A,
> > > hsr_sp->macaddress_A)) {
> > > +                       /* This is a redbox supervision frame for a
> > > VDAN! */
> > > +                       /* Push second TLV and payload here */
> > > +                       skb_push(skb, sizeof(struct
> > > hsr_sup_payload) + sizeof(struct hsr_sup_tlv));
> > > +                       goto done;
> > > +               }
> > > +               /* Push second TLV here */
> > > +               skb_push(skb, sizeof(struct hsr_sup_tlv));
> > > +       }
> > > +       /* Push payload here */
> > > +       skb_push(skb, sizeof(struct hsr_sup_payload));  
> > 
> > Is this code path handling frames from the network or user space? 
> > Does it need input checking?  
> This code path is handling frames from the network. The supervision
> frames are broadcasted by each device. What type of additional input
> checking are you thinking of? I think I should check skb->len before
> pulling/accessing right?

Yes, skb->len would be the start, but skb can also have fragments, so
skb->len > N does not guarantee that N bytes are accessible directly
via skb->data. I think that calling pskb_may_pull() would be the best
fit for your use.

I'm not sure why this function doesn't do it already, perhaps there 
is some guarantee I'm missing.