Re: Checking SPI in xfrm_state_find

[Patrick McHardy <kaber@xxxxxxxxx>]

Herbert Xu wrote:
> On Mon, Mar 28, 2005 at 10:10:54PM +0200, Patrick McHardy wrote:
>
> > Something unrelated I was also wondering about, from xfrm_find_state():
> >
> >        list_for_each_entry(x, xfrm_state_bydst+h, bydst) {
> >                if (x->props.family == family &&
> >                    x->props.reqid == tmpl->reqid &&
> >                    xfrm_state_addr_check(x, daddr, saddr, family) &&
> >                    tmpl->mode == x->props.mode &&
> >                    tmpl->id.proto == x->id.proto) {
> >
> > Shouldn't we check for (tmpl->id.spi == x->id.spi || !tmpl->id.spi) ?
>
> Absolutely.  We should also fix the larval state generation in that
> same function to fail the operation if that SPI already exists.
Thanks, both done by these two patches.

Regards
Patrick

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/30 06:02:45+02:00 kaber@xxxxxxxxxxxx 
#   [IPSEC]: Check SPI in xfrm_state_find()
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
# net/xfrm/xfrm_state.c
#   2005/03/30 06:02:36+02:00 kaber@xxxxxxxxxxxx +2 -1
#   [IPSEC]: Check SPI in xfrm_state_find()
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
diff -Nru a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
--- a/net/xfrm/xfrm_state.c     2005-03-31 02:12:12 +02:00
+++ b/net/xfrm/xfrm_state.c     2005-03-31 02:12:12 +02:00
@@ -306,7 +306,8 @@
                    x->props.reqid == tmpl->reqid &&
                    xfrm_state_addr_check(x, daddr, saddr, family) &&
                    tmpl->mode == x->props.mode &&
-                   tmpl->id.proto == x->id.proto) {
+                   tmpl->id.proto == x->id.proto &&
+                   (tmpl->id.spi == x->id.spi || !tmpl->id.spi)) {
                        /* Resolution logic:
                           1. There is a valid state with matching selector.
                              Done.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/31 02:07:54+02:00 kaber@xxxxxxxxxxxx 
#   [IPSEC]: Check if SPI exists before creating acquire state
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
# net/xfrm/xfrm_state.c
#   2005/03/31 02:07:42+02:00 kaber@xxxxxxxxxxxx +25 -7
#   [IPSEC]: Check if SPI exists before creating acquire state
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
diff -Nru a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
--- a/net/xfrm/xfrm_state.c     2005-03-31 02:12:57 +02:00
+++ b/net/xfrm/xfrm_state.c     2005-03-31 02:12:57 +02:00
@@ -295,10 +295,17 @@
                unsigned short family)
 {
        unsigned h = xfrm_dst_hash(daddr, family);
-       struct xfrm_state *x;
+       struct xfrm_state *x, *x0;
        int acquire_in_progress = 0;
        int error = 0;
        struct xfrm_state *best = NULL;
+       struct xfrm_state_afinfo *afinfo;
+       
+       afinfo = xfrm_state_get_afinfo(family);
+       if (afinfo == NULL) {
+               *err = -EAFNOSUPPORT;
+               return NULL;
+       }
 
        spin_lock_bh(&xfrm_state_lock);
        list_for_each_entry(x, xfrm_state_bydst+h, bydst) {
@@ -334,14 +341,24 @@
                        } else if (x->km.state == XFRM_STATE_ERROR ||
                                   x->km.state == XFRM_STATE_EXPIRED) {
                                if (xfrm_selector_match(&x->sel, fl, family))
-                                       error = 1;
+                                       error = -ESRCH;
                        }
                }
        }
 
        x = best;
-       if (!x && !error && !acquire_in_progress &&
-           ((x = xfrm_state_alloc()) != NULL)) {
+       if (!x && !error && !acquire_in_progress) {
+               x0 = afinfo->state_lookup(&tmpl->id.daddr, tmpl->id.spi, 
tmpl->id.proto);
+               if (x0 != NULL) {
+                       xfrm_state_put(x0);
+                       error = -EEXIST;
+                       goto out;
+               }
+               x = xfrm_state_alloc();
+               if (x == NULL) {
+                       error = -ENOMEM;
+                       goto out;
+               }
                /* Initialize temporary selector matching only
                 * to current session. */
                xfrm_init_tempsel(x, fl, tmpl, daddr, saddr, family);
@@ -363,15 +380,16 @@
                        x->km.state = XFRM_STATE_DEAD;
                        xfrm_state_put(x);
                        x = NULL;
-                       error = 1;
+                       error = -ESRCH;
                }
        }
+out:
        if (x)
                xfrm_state_hold(x);
        else
-               *err = acquire_in_progress ? -EAGAIN :
-                       (error ? -ESRCH : -ENOMEM);
+               *err = acquire_in_progress ? -EAGAIN : error;
        spin_unlock_bh(&xfrm_state_lock);
+       xfrm_state_put_afinfo(afinfo);
        return x;
 }