netdev - Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

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]

Date:   Tue, 22 Nov 2022 15:57:43 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

On Tue, Nov 22, 2022 at 02:10:02PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > > 
> > > > I think that something like this will do the trick.
> > > > 
> > > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > > index 5076f9d7a752..d1c9ef857755 100644
> > > > --- a/net/xfrm/xfrm_state.c
> > > > +++ b/net/xfrm/xfrm_state.c
> > > > @@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
> > > >         }
> > > >  }
> > > > 
> > > > +static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
> > > > +                                       struct xfrm_policy *p)
> > > > +{
> > > > +       /* Packet offload: both policy and SA should be offloaded */
> > > > +       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > > +           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return true;
> > > > +
> > > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > > > +           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return true;
> > > > +
> > > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return false;
> > > > +
> > > > +       /* Packet offload: both policy and SA should have same device */
> > > > +       if (p->xdo.dev != x->xso.dev)
> > > > +               return true;
> > > > +
> > > > +       return false;
> > > > +}
> > > > +
> > > >  struct xfrm_state *
> > > >  xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > >                 const struct flowi *fl, struct xfrm_tmpl *tmpl,
> > > > @@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > > 
> > > >  found:
> > > >         x = best;
> > > > -       if (!x && !error && !acquire_in_progress) {
> > > > +       if (!x && !error && !acquire_in_progress &&
> > > > +           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
> > > >                 if (tmpl->id.spi &&
> > > >                     (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
> > > >                                               tmpl->id.proto, encap_family)) != NULL) {
> > > > @@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > >                         *err = -EAGAIN;
> > > >                         x = NULL;
> > > >                 }
> > > > +               if (x && xfrm_state_and_policy_mixed(x, pol)) {
> > > > +                       *err = -EINVAL;
> > > > +                       x = NULL;
> > > 
> > > If policy and state do not match here, this means the lookup
> > > returned the wrong state. The correct state might still sit
> > > in the database. At this point, you should either have found
> > > a matching state, or no state at all.
> > 
> > I check for "x" because of "x = NULL" above.
> 
> This does not change the fact that the lookup returned the wrong state.

Steffen, but this is exactly why we added this check - to catch wrong
states and configurations. 

If lookup was successful, we know that HW handles this packet, if lookup
failed to find SA and we have HW policy, we should drop such packet.

Thanks