diff mbox series

examples/ipsec-secgw: fix SPD no-match is misinterpreted

Message ID 20190328124733.25580-1-konstantin.ananyev@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series examples/ipsec-secgw: fix SPD no-match is misinterpreted |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing fail Performance Testing issues
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Ananyev, Konstantin March 28, 2019, 12:47 p.m. UTC 
  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.

Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
Fixes: 2a5106af132b ("examples/ipsec-secgw: fix corner case for SPI value")
Cc: stable@dpdk.org

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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(-)

Comments

Akhil Goyal March 29, 2019, 10:53 a.m. UTC | #1 
Hi Konstantin,

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.

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: stable@dpdk.org
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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;
>   	}
>
Ananyev, Konstantin March 29, 2019, 6:22 p.m. UTC | #2 
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: stable@dpdk.org
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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;
> >   	}
> >
Ananyev, Konstantin March 30, 2019, 11:22 a.m. UTC | #3 
> 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

After another thought, it seems that we can easily overcome that problem
without introducing extra limitations - just need to store 'sa_idx + 1' in acl table.
Will give it a try with v2.
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: stable@dpdk.org
> > >
> > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@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;
> > >   	}
> > >
Ananyev, Konstantin April 4, 2019, 12:16 p.m. UTC | #4 
Hi Akhil, 

> > > > 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
> 
> After another thought, it seems that we can easily overcome that problem
> without introducing extra limitations - just need to store 'sa_idx + 1' in acl table.
> Will give it a try with v2.

Actually, after yet another thought - I think there is no need for that.
We can store in ACL full SPI and do SPI2IDX after acl_classify().
Just sent v2, please have a look.
Konstantin
diff mbox series

Patch

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;
 	}