Hi Akhil,
>
> On 3/28/2019 6:17 PM, Konstantin Ananyev wrote:
> > acl_classify() returns zero value when no matching rule was found.
> > Currently ipsec-secgw treats it as a valid SPI value, though it has
> > to discard such packets.
> > Error could be easily observed by sending outbound unmatched packets,
> > user will see something like that in the log:
> > IPSEC: No cryptodev: core 7, cipher_algo 0, auth_algo 0, aead_algo 0
> >
> > To fix it we need to treat packets with zero result from acl_classify()
> > as invalid ones. Also we can change DISCARD and BYPASS values to
> > simplify checks and save some extra space for valid SPI values.
> spi value =0 is invalid but zero result may have a valid packet.
> consider a case:
> SPI = 128 or 256 or 512 and so on => sa_idx = 0 and result will come as
> zero, and this would be a valid packet.
>
> I see that the sa_idx calculation logic is not correct in first place.
> There will be multiple spi values for same sa_idx which is not correct.
> So we have 2 issues here:
> 1. result = 0, means sa_idx =0 which may be correct, but as you said if
> acl_classify fails, it also return 0.
> 2. SPI values which are IPSEC_SA_MAX_ENTRIES apart will have same sa_idx
> and will keep on overwriting the previous ones.
>
Ok I see what you mean.
The easiest fix for that (till we'll have proper SAD) would be not to allow
SPIs bigger than IPSEC_SA_MAX_ENTRIES.
Are you ok with that?
Konstantin
> So I believe the fix in this patch is not enough to resolve these
> issues. It will work on some values and will break on other values of spi.
>
> -Akhil
>
> >
> > Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
> > Fixes: 2a5106af132b ("examples/ipsec-secgw: fix corner case for SPI value")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.anan...@intel.com>
> > ---
> > examples/ipsec-secgw/ipsec-secgw.c | 12 ++++++------
> > examples/ipsec-secgw/ipsec.h | 6 ++----
> > examples/ipsec-secgw/sp4.c | 11 ++++++++---
> > examples/ipsec-secgw/sp6.c | 11 ++++++++---
> > 4 files changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> > b/examples/ipsec-secgw/ipsec-secgw.c
> > index ffbd00b08..59e084234 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.c
> > +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > @@ -438,11 +438,11 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa,
> > struct traffic_type *ip,
> > for (i = 0; i < ip->num; i++) {
> > m = ip->pkts[i];
> > res = ip->res[i];
> > - if (res & BYPASS) {
> > + if (res == BYPASS) {
> > ip->pkts[j++] = m;
> > continue;
> > }
> > - if (res & DISCARD) {
> > + if (res == DISCARD) {
> > rte_pktmbuf_free(m);
> > continue;
> > }
> > @@ -453,7 +453,7 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa,
> > struct traffic_type *ip,
> > continue;
> > }
> >
> > - sa_idx = ip->res[i] & PROTECT_MASK;
> > + sa_idx = ip->res[i];
> > if (sa_idx >= IPSEC_SA_MAX_ENTRIES ||
> > !inbound_sa_check(sa, m, sa_idx)) {
> > rte_pktmbuf_free(m);
> > @@ -541,10 +541,10 @@ outbound_sp(struct sp_ctx *sp, struct traffic_type
> > *ip,
> > j = 0;
> > for (i = 0; i < ip->num; i++) {
> > m = ip->pkts[i];
> > - sa_idx = ip->res[i] & PROTECT_MASK;
> > - if (ip->res[i] & DISCARD)
> > + sa_idx = ip->res[i];
> > + if (sa_idx == DISCARD)
> > rte_pktmbuf_free(m);
> > - else if (ip->res[i] & BYPASS)
> > + else if (sa_idx == BYPASS)
> > ip->pkts[j++] = m;
> > else if (sa_idx < IPSEC_SA_MAX_ENTRIES) {
> > ipsec->res[ipsec->num] = sa_idx;
> > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> > index 99f49d65f..44daf384b 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -41,10 +41,8 @@
> > #define SPI2IDX(spi) (spi & (IPSEC_SA_MAX_ENTRIES - 1))
> > #define INVALID_SPI (0)
> >
> > -#define DISCARD (0x80000000)
> > -#define BYPASS (0x40000000)
> > -#define PROTECT_MASK (0x3fffffff)
> > -#define PROTECT(sa_idx) (SPI2IDX(sa_idx) & PROTECT_MASK) /* SA idx 30 bits
> > */
> > +#define DISCARD INVALID_SPI
> > +#define BYPASS UINT32_MAX
> >
> > #define IPSEC_XFORM_MAX 2
> >
> > diff --git a/examples/ipsec-secgw/sp4.c b/examples/ipsec-secgw/sp4.c
> > index d1dc64bad..bfaddc52e 100644
> > --- a/examples/ipsec-secgw/sp4.c
> > +++ b/examples/ipsec-secgw/sp4.c
> > @@ -99,6 +99,7 @@ parse_sp4_tokens(char **tokens, uint32_t n_tokens,
> >
> > uint32_t *ri = NULL; /* rule index */
> > uint32_t ti = 0; /* token index */
> > + uint32_t tv;
> >
> > uint32_t esp_p = 0;
> > uint32_t protect_p = 0;
> > @@ -169,8 +170,12 @@ parse_sp4_tokens(char **tokens, uint32_t n_tokens,
> > if (status->status < 0)
> > return;
> >
> > - rule_ipv4->data.userdata =
> > - PROTECT(atoi(tokens[ti]));
> > + tv = atoi(tokens[ti]);
> > + APP_CHECK(tv != DISCARD && tv != BYPASS, status,
> > + "invalid SPI: %s", tokens[ti]);
> > + if (status->status < 0)
> > + return;
> > + rule_ipv4->data.userdata = tv;
> >
> > protect_p = 1;
> > continue;
> > @@ -523,7 +528,7 @@ sp4_spi_present(uint32_t spi, int inbound)
> > }
> >
> > for (i = 0; i != num; i++) {
> > - if (acr[i].data.userdata == PROTECT(spi))
> > + if (acr[i].data.userdata == spi)
> > return i;
> > }
> >
> > diff --git a/examples/ipsec-secgw/sp6.c b/examples/ipsec-secgw/sp6.c
> > index e67d85aaf..b7fcf7c16 100644
> > --- a/examples/ipsec-secgw/sp6.c
> > +++ b/examples/ipsec-secgw/sp6.c
> > @@ -130,6 +130,7 @@ parse_sp6_tokens(char **tokens, uint32_t n_tokens,
> >
> > uint32_t *ri = NULL; /* rule index */
> > uint32_t ti = 0; /* token index */
> > + uint32_t tv;
> >
> > uint32_t esp_p = 0;
> > uint32_t protect_p = 0;
> > @@ -202,8 +203,12 @@ parse_sp6_tokens(char **tokens, uint32_t n_tokens,
> > if (status->status < 0)
> > return;
> >
> > - rule_ipv6->data.userdata =
> > - PROTECT(atoi(tokens[ti]));
> > + tv = atoi(tokens[ti]);
> > + APP_CHECK(tv != DISCARD && tv != BYPASS, status,
> > + "invalid SPI: %s", tokens[ti]);
> > + if (status->status < 0)
> > + return;
> > + rule_ipv6->data.userdata = tv;
> >
> > protect_p = 1;
> > continue;
> > @@ -637,7 +642,7 @@ sp6_spi_present(uint32_t spi, int inbound)
> > }
> >
> > for (i = 0; i != num; i++) {
> > - if (acr[i].data.userdata == PROTECT(spi))
> > + if (acr[i].data.userdata == spi)
> > return i;
> > }
> >
