* [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
@ 2019-03-28 12:47 Konstantin Ananyev
2019-03-28 12:47 ` Konstantin Ananyev
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Konstantin Ananyev @ 2019-03-28 12:47 UTC (permalink / raw)
To: dev; +Cc: akhil.goyal, Konstantin Ananyev, stable
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(-)
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;
}
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-28 12:47 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted Konstantin Ananyev
@ 2019-03-28 12:47 ` Konstantin Ananyev
2019-03-29 10:53 ` Akhil Goyal
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
2 siblings, 0 replies; 16+ messages in thread
From: Konstantin Ananyev @ 2019-03-28 12:47 UTC (permalink / raw)
To: dev; +Cc: akhil.goyal, Konstantin Ananyev, stable
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(-)
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;
}
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-28 12:47 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted Konstantin Ananyev
2019-03-28 12:47 ` Konstantin Ananyev
@ 2019-03-29 10:53 ` Akhil Goyal
2019-03-29 10:53 ` Akhil Goyal
2019-03-29 18:22 ` Ananyev, Konstantin
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
2 siblings, 2 replies; 16+ messages in thread
From: Akhil Goyal @ 2019-03-29 10:53 UTC (permalink / raw)
To: Konstantin Ananyev, dev; +Cc: stable
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;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-29 10:53 ` Akhil Goyal
@ 2019-03-29 10:53 ` Akhil Goyal
2019-03-29 18:22 ` Ananyev, Konstantin
1 sibling, 0 replies; 16+ messages in thread
From: Akhil Goyal @ 2019-03-29 10:53 UTC (permalink / raw)
To: Konstantin Ananyev, dev; +Cc: stable
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;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-29 10:53 ` Akhil Goyal
2019-03-29 10:53 ` Akhil Goyal
@ 2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
1 sibling, 2 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-03-29 18:22 UTC (permalink / raw)
To: Akhil Goyal, dev; +Cc: stable
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;
> > }
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-29 18:22 ` Ananyev, Konstantin
@ 2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
1 sibling, 0 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-03-29 18:22 UTC (permalink / raw)
To: Akhil Goyal, dev; +Cc: stable
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;
> > }
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-29 18:22 ` Ananyev, Konstantin
@ 2019-03-30 11:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
1 sibling, 2 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-03-30 11:22 UTC (permalink / raw)
To: Ananyev, Konstantin, Akhil Goyal, dev; +Cc: stable
> 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;
> > > }
> > >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-30 11:22 ` Ananyev, Konstantin
@ 2019-03-30 11:22 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
1 sibling, 0 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-03-30 11:22 UTC (permalink / raw)
To: Ananyev, Konstantin, Akhil Goyal, dev; +Cc: stable
> 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;
> > > }
> > >
^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-28 12:47 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted Konstantin Ananyev
2019-03-28 12:47 ` Konstantin Ananyev
2019-03-29 10:53 ` Akhil Goyal
@ 2019-04-04 12:13 ` Konstantin Ananyev
2019-04-04 12:13 ` Konstantin Ananyev
2019-04-04 18:39 ` Zhang, Roy Fan
2 siblings, 2 replies; 16+ messages in thread
From: Konstantin Ananyev @ 2019-04-04 12:13 UTC (permalink / raw)
To: dev; +Cc: akhil.goyal, Konstantin Ananyev, stable
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.
To summarize the approach:
1. have special SPI values for DISCARD and BYPASS.
2. store in SPD full SPI value.
3. after acl_classify(), first check SPI value for DISCARD and BYPASS,
then convert it in SA index.
4. add check at initilisation time that for each SPD rule there is a
corresponding SA entry (with the same SPI).
Also marked few global variables as *static*.
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 | 20 ++++++------
examples/ipsec-secgw/ipsec.h | 14 ++++++---
examples/ipsec-secgw/sa.c | 35 ++++++++++++++++++---
examples/ipsec-secgw/sp4.c | 49 ++++++++++++++++++++++++++++--
examples/ipsec-secgw/sp6.c | 49 ++++++++++++++++++++++++++++--
5 files changed, 141 insertions(+), 26 deletions(-)
diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index ffbd00b08..2e203393d 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,9 +453,8 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type *ip,
continue;
}
- sa_idx = ip->res[i] & PROTECT_MASK;
- if (sa_idx >= IPSEC_SA_MAX_ENTRIES ||
- !inbound_sa_check(sa, m, sa_idx)) {
+ sa_idx = SPI2IDX(res);
+ if (!inbound_sa_check(sa, m, sa_idx)) {
rte_pktmbuf_free(m);
continue;
}
@@ -541,16 +540,15 @@ 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 = SPI2IDX(ip->res[i]);
+ if (ip->res[i] == DISCARD)
rte_pktmbuf_free(m);
- else if (ip->res[i] & BYPASS)
+ else if (ip->res[i] == BYPASS)
ip->pkts[j++] = m;
- else if (sa_idx < IPSEC_SA_MAX_ENTRIES) {
+ else {
ipsec->res[ipsec->num] = sa_idx;
ipsec->pkts[ipsec->num++] = m;
- } else /* invalid SA idx */
- rte_pktmbuf_free(m);
+ }
}
ip->num = j;
}
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 99f49d65f..589398f6f 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
@@ -289,6 +287,14 @@ sp4_spi_present(uint32_t spi, int inbound);
int
sp6_spi_present(uint32_t spi, int inbound);
+/*
+ * Search through SA entries for given SPI.
+ * Returns first entry index if found(greater or equal then zero),
+ * or -ENOENT otherwise.
+ */
+int
+sa_spi_present(uint32_t spi, int inbound);
+
void
sa_init(struct socket_ctx *ctx, int32_t socket_id);
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index a7298a30c..b850e9839 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -126,11 +126,11 @@ const struct supported_aead_algo aead_algos[] = {
}
};
-struct ipsec_sa sa_out[IPSEC_SA_MAX_ENTRIES];
-uint32_t nb_sa_out;
+static struct ipsec_sa sa_out[IPSEC_SA_MAX_ENTRIES];
+static uint32_t nb_sa_out;
-struct ipsec_sa sa_in[IPSEC_SA_MAX_ENTRIES];
-uint32_t nb_sa_in;
+static struct ipsec_sa sa_in[IPSEC_SA_MAX_ENTRIES];
+static uint32_t nb_sa_in;
static const struct supported_cipher_algo *
find_match_cipher_algo(const char *cipher_keyword)
@@ -631,7 +631,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
*ri = *ri + 1;
}
-static inline void
+static void
print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
{
uint32_t i;
@@ -1116,6 +1116,31 @@ ipsec_satbl_init(struct sa_ctx *ctx, const struct ipsec_sa *ent,
return rc;
}
+/*
+ * Walk through all SA rules to find an SA with given SPI
+ */
+int
+sa_spi_present(uint32_t spi, int inbound)
+{
+ uint32_t i, num;
+ const struct ipsec_sa *sar;
+
+ if (inbound != 0) {
+ sar = sa_in;
+ num = nb_sa_in;
+ } else {
+ sar = sa_out;
+ num = nb_sa_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ if (sar[i].spi == spi)
+ return i;
+ }
+
+ return -ENOENT;
+}
+
void
sa_init(struct socket_ctx *ctx, int32_t socket_id)
{
diff --git a/examples/ipsec-secgw/sp4.c b/examples/ipsec-secgw/sp4.c
index d1dc64bad..ca9ee7f24 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;
@@ -472,6 +477,36 @@ acl4_init(const char *name, int32_t socketid, const struct acl4_rules *rules,
return ctx;
}
+/*
+ * check that for each rule it's SPI has a correspondent entry in SAD
+ */
+static int
+check_spi_value(int inbound)
+{
+ uint32_t i, num, spi;
+ const struct acl4_rules *acr;
+
+ if (inbound != 0) {
+ acr = acl4_rules_in;
+ num = nb_acl4_rules_in;
+ } else {
+ acr = acl4_rules_out;
+ num = nb_acl4_rules_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ spi = acr[i].data.userdata;
+ if (spi != DISCARD && spi != BYPASS &&
+ sa_spi_present(spi, inbound) < 0) {
+ RTE_LOG(ERR, IPSEC, "SPI %u is not present in SAD\n",
+ spi);
+ return -ENOENT;
+ }
+ }
+
+ return 0;
+}
+
void
sp4_init(struct socket_ctx *ctx, int32_t socket_id)
{
@@ -488,6 +523,14 @@ sp4_init(struct socket_ctx *ctx, int32_t socket_id)
rte_exit(EXIT_FAILURE, "Outbound SP DB for socket %u already "
"initialized\n", socket_id);
+ if (check_spi_value(1) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Inbound IPv4 SP DB has unmatched in SAD SPIs\n");
+
+ if (check_spi_value(0) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Outbound IPv4 SP DB has unmatched in SAD SPIs\n");
+
if (nb_acl4_rules_in > 0) {
name = "sp_ip4_in";
ctx->sp_ip4_in = (struct sp_ctx *)acl4_init(name,
@@ -523,7 +566,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..76be3d3e9 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;
@@ -586,6 +591,36 @@ acl6_init(const char *name, int32_t socketid, const struct acl6_rules *rules,
return ctx;
}
+/*
+ * check that for each rule it's SPI has a correspondent entry in SAD
+ */
+static int
+check_spi_value(int inbound)
+{
+ uint32_t i, num, spi;
+ const struct acl6_rules *acr;
+
+ if (inbound != 0) {
+ acr = acl6_rules_in;
+ num = nb_acl6_rules_in;
+ } else {
+ acr = acl6_rules_out;
+ num = nb_acl6_rules_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ spi = acr[i].data.userdata;
+ if (spi != DISCARD && spi != BYPASS &&
+ sa_spi_present(spi, inbound) < 0) {
+ RTE_LOG(ERR, IPSEC, "SPI %u is not present in SAD\n",
+ spi);
+ return -ENOENT;
+ }
+ }
+
+ return 0;
+}
+
void
sp6_init(struct socket_ctx *ctx, int32_t socket_id)
{
@@ -602,6 +637,14 @@ sp6_init(struct socket_ctx *ctx, int32_t socket_id)
rte_exit(EXIT_FAILURE, "Outbound IPv6 SP DB for socket %u "
"already initialized\n", socket_id);
+ if (check_spi_value(1) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Inbound IPv6 SP DB has unmatched in SAD SPIs\n");
+
+ if (check_spi_value(0) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Outbound IPv6 SP DB has unmatched in SAD SPIs\n");
+
if (nb_acl6_rules_in > 0) {
name = "sp_ip6_in";
ctx->sp_ip6_in = (struct sp_ctx *)acl6_init(name,
@@ -637,7 +680,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;
}
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
@ 2019-04-04 12:13 ` Konstantin Ananyev
2019-04-04 18:39 ` Zhang, Roy Fan
1 sibling, 0 replies; 16+ messages in thread
From: Konstantin Ananyev @ 2019-04-04 12:13 UTC (permalink / raw)
To: dev; +Cc: akhil.goyal, Konstantin Ananyev, stable
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.
To summarize the approach:
1. have special SPI values for DISCARD and BYPASS.
2. store in SPD full SPI value.
3. after acl_classify(), first check SPI value for DISCARD and BYPASS,
then convert it in SA index.
4. add check at initilisation time that for each SPD rule there is a
corresponding SA entry (with the same SPI).
Also marked few global variables as *static*.
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 | 20 ++++++------
examples/ipsec-secgw/ipsec.h | 14 ++++++---
examples/ipsec-secgw/sa.c | 35 ++++++++++++++++++---
examples/ipsec-secgw/sp4.c | 49 ++++++++++++++++++++++++++++--
examples/ipsec-secgw/sp6.c | 49 ++++++++++++++++++++++++++++--
5 files changed, 141 insertions(+), 26 deletions(-)
diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
index ffbd00b08..2e203393d 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,9 +453,8 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type *ip,
continue;
}
- sa_idx = ip->res[i] & PROTECT_MASK;
- if (sa_idx >= IPSEC_SA_MAX_ENTRIES ||
- !inbound_sa_check(sa, m, sa_idx)) {
+ sa_idx = SPI2IDX(res);
+ if (!inbound_sa_check(sa, m, sa_idx)) {
rte_pktmbuf_free(m);
continue;
}
@@ -541,16 +540,15 @@ 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 = SPI2IDX(ip->res[i]);
+ if (ip->res[i] == DISCARD)
rte_pktmbuf_free(m);
- else if (ip->res[i] & BYPASS)
+ else if (ip->res[i] == BYPASS)
ip->pkts[j++] = m;
- else if (sa_idx < IPSEC_SA_MAX_ENTRIES) {
+ else {
ipsec->res[ipsec->num] = sa_idx;
ipsec->pkts[ipsec->num++] = m;
- } else /* invalid SA idx */
- rte_pktmbuf_free(m);
+ }
}
ip->num = j;
}
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 99f49d65f..589398f6f 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
@@ -289,6 +287,14 @@ sp4_spi_present(uint32_t spi, int inbound);
int
sp6_spi_present(uint32_t spi, int inbound);
+/*
+ * Search through SA entries for given SPI.
+ * Returns first entry index if found(greater or equal then zero),
+ * or -ENOENT otherwise.
+ */
+int
+sa_spi_present(uint32_t spi, int inbound);
+
void
sa_init(struct socket_ctx *ctx, int32_t socket_id);
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index a7298a30c..b850e9839 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -126,11 +126,11 @@ const struct supported_aead_algo aead_algos[] = {
}
};
-struct ipsec_sa sa_out[IPSEC_SA_MAX_ENTRIES];
-uint32_t nb_sa_out;
+static struct ipsec_sa sa_out[IPSEC_SA_MAX_ENTRIES];
+static uint32_t nb_sa_out;
-struct ipsec_sa sa_in[IPSEC_SA_MAX_ENTRIES];
-uint32_t nb_sa_in;
+static struct ipsec_sa sa_in[IPSEC_SA_MAX_ENTRIES];
+static uint32_t nb_sa_in;
static const struct supported_cipher_algo *
find_match_cipher_algo(const char *cipher_keyword)
@@ -631,7 +631,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
*ri = *ri + 1;
}
-static inline void
+static void
print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
{
uint32_t i;
@@ -1116,6 +1116,31 @@ ipsec_satbl_init(struct sa_ctx *ctx, const struct ipsec_sa *ent,
return rc;
}
+/*
+ * Walk through all SA rules to find an SA with given SPI
+ */
+int
+sa_spi_present(uint32_t spi, int inbound)
+{
+ uint32_t i, num;
+ const struct ipsec_sa *sar;
+
+ if (inbound != 0) {
+ sar = sa_in;
+ num = nb_sa_in;
+ } else {
+ sar = sa_out;
+ num = nb_sa_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ if (sar[i].spi == spi)
+ return i;
+ }
+
+ return -ENOENT;
+}
+
void
sa_init(struct socket_ctx *ctx, int32_t socket_id)
{
diff --git a/examples/ipsec-secgw/sp4.c b/examples/ipsec-secgw/sp4.c
index d1dc64bad..ca9ee7f24 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;
@@ -472,6 +477,36 @@ acl4_init(const char *name, int32_t socketid, const struct acl4_rules *rules,
return ctx;
}
+/*
+ * check that for each rule it's SPI has a correspondent entry in SAD
+ */
+static int
+check_spi_value(int inbound)
+{
+ uint32_t i, num, spi;
+ const struct acl4_rules *acr;
+
+ if (inbound != 0) {
+ acr = acl4_rules_in;
+ num = nb_acl4_rules_in;
+ } else {
+ acr = acl4_rules_out;
+ num = nb_acl4_rules_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ spi = acr[i].data.userdata;
+ if (spi != DISCARD && spi != BYPASS &&
+ sa_spi_present(spi, inbound) < 0) {
+ RTE_LOG(ERR, IPSEC, "SPI %u is not present in SAD\n",
+ spi);
+ return -ENOENT;
+ }
+ }
+
+ return 0;
+}
+
void
sp4_init(struct socket_ctx *ctx, int32_t socket_id)
{
@@ -488,6 +523,14 @@ sp4_init(struct socket_ctx *ctx, int32_t socket_id)
rte_exit(EXIT_FAILURE, "Outbound SP DB for socket %u already "
"initialized\n", socket_id);
+ if (check_spi_value(1) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Inbound IPv4 SP DB has unmatched in SAD SPIs\n");
+
+ if (check_spi_value(0) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Outbound IPv4 SP DB has unmatched in SAD SPIs\n");
+
if (nb_acl4_rules_in > 0) {
name = "sp_ip4_in";
ctx->sp_ip4_in = (struct sp_ctx *)acl4_init(name,
@@ -523,7 +566,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..76be3d3e9 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;
@@ -586,6 +591,36 @@ acl6_init(const char *name, int32_t socketid, const struct acl6_rules *rules,
return ctx;
}
+/*
+ * check that for each rule it's SPI has a correspondent entry in SAD
+ */
+static int
+check_spi_value(int inbound)
+{
+ uint32_t i, num, spi;
+ const struct acl6_rules *acr;
+
+ if (inbound != 0) {
+ acr = acl6_rules_in;
+ num = nb_acl6_rules_in;
+ } else {
+ acr = acl6_rules_out;
+ num = nb_acl6_rules_out;
+ }
+
+ for (i = 0; i != num; i++) {
+ spi = acr[i].data.userdata;
+ if (spi != DISCARD && spi != BYPASS &&
+ sa_spi_present(spi, inbound) < 0) {
+ RTE_LOG(ERR, IPSEC, "SPI %u is not present in SAD\n",
+ spi);
+ return -ENOENT;
+ }
+ }
+
+ return 0;
+}
+
void
sp6_init(struct socket_ctx *ctx, int32_t socket_id)
{
@@ -602,6 +637,14 @@ sp6_init(struct socket_ctx *ctx, int32_t socket_id)
rte_exit(EXIT_FAILURE, "Outbound IPv6 SP DB for socket %u "
"already initialized\n", socket_id);
+ if (check_spi_value(1) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Inbound IPv6 SP DB has unmatched in SAD SPIs\n");
+
+ if (check_spi_value(0) < 0)
+ rte_exit(EXIT_FAILURE,
+ "Outbound IPv6 SP DB has unmatched in SAD SPIs\n");
+
if (nb_acl6_rules_in > 0) {
name = "sp_ip6_in";
ctx->sp_ip6_in = (struct sp_ctx *)acl6_init(name,
@@ -637,7 +680,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;
}
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-03-30 11:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
@ 2019-04-04 12:16 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
1 sibling, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-04-04 12:16 UTC (permalink / raw)
To: 'Akhil Goyal', 'dev@dpdk.org'; +Cc: 'stable@dpdk.org'
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-04 12:16 ` Ananyev, Konstantin
@ 2019-04-04 12:16 ` Ananyev, Konstantin
0 siblings, 0 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2019-04-04 12:16 UTC (permalink / raw)
To: 'Akhil Goyal', 'dev@dpdk.org'; +Cc: 'stable@dpdk.org'
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
2019-04-04 12:13 ` Konstantin Ananyev
@ 2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-23 12:58 ` Akhil Goyal
1 sibling, 2 replies; 16+ messages in thread
From: Zhang, Roy Fan @ 2019-04-04 18:39 UTC (permalink / raw)
To: Ananyev, Konstantin, dev; +Cc: akhil.goyal, Ananyev, Konstantin, stable
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin
> Ananyev
> Sent: Thursday, April 4, 2019 1:13 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> misinterpreted
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-04 18:39 ` Zhang, Roy Fan
@ 2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-23 12:58 ` Akhil Goyal
1 sibling, 0 replies; 16+ messages in thread
From: Zhang, Roy Fan @ 2019-04-04 18:39 UTC (permalink / raw)
To: Ananyev, Konstantin, dev; +Cc: akhil.goyal, Ananyev, Konstantin, stable
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin
> Ananyev
> Sent: Thursday, April 4, 2019 1:13 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> misinterpreted
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-04 18:39 ` Zhang, Roy Fan
@ 2019-04-23 12:58 ` Akhil Goyal
2019-04-23 12:58 ` Akhil Goyal
1 sibling, 1 reply; 16+ messages in thread
From: Akhil Goyal @ 2019-04-23 12:58 UTC (permalink / raw)
To: Zhang, Roy Fan, Ananyev, Konstantin, dev; +Cc: Ananyev, Konstantin, stable
> -----Original Message-----
> From: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Sent: Friday, April 5, 2019 12:10 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> misinterpreted
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin
> > Ananyev
> > Sent: Thursday, April 4, 2019 1:13 PM
> > To: dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> > misinterpreted
>
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Applied to dpdk-next-crypto
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is misinterpreted
2019-04-23 12:58 ` Akhil Goyal
@ 2019-04-23 12:58 ` Akhil Goyal
0 siblings, 0 replies; 16+ messages in thread
From: Akhil Goyal @ 2019-04-23 12:58 UTC (permalink / raw)
To: Zhang, Roy Fan, Ananyev, Konstantin, dev; +Cc: Ananyev, Konstantin, stable
> -----Original Message-----
> From: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Sent: Friday, April 5, 2019 12:10 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> misinterpreted
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin
> > Ananyev
> > Sent: Thursday, April 4, 2019 1:13 PM
> > To: dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: fix SPD no-match is
> > misinterpreted
>
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Applied to dpdk-next-crypto
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-04-23 12:58 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-28 12:47 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted Konstantin Ananyev
2019-03-28 12:47 ` Konstantin Ananyev
2019-03-29 10:53 ` Akhil Goyal
2019-03-29 10:53 ` Akhil Goyal
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
2019-04-04 12:13 ` Konstantin Ananyev
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-23 12:58 ` Akhil Goyal
2019-04-23 12:58 ` Akhil Goyal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).