[dpdk-dev] [PATCH] net: check that seg is valid before dereference

[dpdk-dev] [PATCH] net: check that seg is valid before dereference

[Chas Williams]

If the overall pkt_len and segment lengths are out of agreement,
it is possible for the seg to be NULL after the loop. Add assert
to check this condition in debug builds.

Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 lib/librte_net/rte_ip.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index fcd1eb342..6b3e4cdda 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len,
 			break;
 		off -= seglen;
 	}
+	RTE_ASSERT(seg != NULL);
 	seglen -= off;
 	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
 	if (seglen >= len) {
-- 
2.26.2

Re: [dpdk-dev] [PATCH] net: check that seg is valid before dereference

[wangyunjian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> Sent: Monday, September 28, 2020 11:32 PM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; Chas Williams <3chas3@gmail.com>
> Subject: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
> 
> If the overall pkt_len and segment lengths are out of agreement, it is possible
> for the seg to be NULL after the loop. Add assert to check this condition in
> debug builds.
> 
> Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")
> 
> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
>  lib/librte_net/rte_ip.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index
> fcd1eb342..6b3e4cdda 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m,
> uint32_t off, uint32_t len,
>  			break;
>  		off -= seglen;
>  	}
> +	RTE_ASSERT(seg != NULL);

Is it better to return an error code?

>  	seglen -= off;
>  	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
>  	if (seglen >= len) {
> --
> 2.26.2

Re: [dpdk-dev] [PATCH] net: check that seg is valid before dereference

[Chas Williams]

On 9/28/20 11:01 PM, wangyunjian wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> Sent: Monday, September 28, 2020 11:32 PM
>> To: dev@dpdk.org
>> Cc: olivier.matz@6wind.com; Chas Williams <3chas3@gmail.com>
>> Subject: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
>>
>> If the overall pkt_len and segment lengths are out of agreement, it is possible
>> for the seg to be NULL after the loop. Add assert to check this condition in
>> debug builds.
>>
>> Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")
>>
>> Signed-off-by: Chas Williams <3chas3@gmail.com>
>> ---
>>   lib/librte_net/rte_ip.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index
>> fcd1eb342..6b3e4cdda 100644
>> --- a/lib/librte_net/rte_ip.h
>> +++ b/lib/librte_net/rte_ip.h
>> @@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m,
>> uint32_t off, uint32_t len,
>>                      break;
>>              off -= seglen;
>>      }
>> +    RTE_ASSERT(seg != NULL);
>
> Is it better to return an error code?

Maybe. However, to get into this state your mbuf chain is already badly broken.
Personally, I would prefer an immediate failure in the application so
I can track
down the source of this error.

No one is checking the return code now, so returning one wouldn't
really help unless I also fix the callers. Lastly, would everyone
be happy with an extra branch in the virtio RX path?


>
>>      seglen -= off;
>>      buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
>>      if (seglen >= len) {
>> --
>> 2.26.2
>

[dpdk-dev] [PATCH v2 1/2] net: check that seg is valid before dereference

[Chas Williams]

If the overall pkt_len and segment lengths are out of agreement,
it is possible for the seg to be NULL after the loop. Add assert
to check this condition in debug builds. Otherwise, return failure.

Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 lib/librte_net/rte_ip.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index fcd1eb342..b2bb5d7b9 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -225,6 +225,9 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len,
 			break;
 		off -= seglen;
 	}
+	RTE_ASSERT(seg != NULL);
+	if (seg == NULL)
+		return -1;
 	seglen -= off;
 	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
 	if (seglen >= len) {
-- 
2.26.2

[dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf

[Chas Williams]

rte_raw_cksum_mbuf can fail, so we should check to see if it
has. If so, return with an error.

Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 drivers/net/virtio/virtio_rxtx.c            | 5 +++--
 drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 0ade35215..019187bf9 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -910,9 +910,10 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 			 */
 			uint16_t csum = 0, off;
 
-			rte_raw_cksum_mbuf(m, hdr->csum_start,
+			if (rte_raw_cksum_mbuf(m, hdr->csum_start,
 				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
-				&csum);
+				&csum) < 0)
+				return -EINVAL;
 			if (likely(csum != 0xffff))
 				csum = ~csum;
 			off = hdr->csum_offset + hdr->csum_start;
diff --git a/drivers/net/virtio/virtio_rxtx_packed_avx.c b/drivers/net/virtio/virtio_rxtx_packed_avx.c
index 6a8214725..67a55abce 100644
--- a/drivers/net/virtio/virtio_rxtx_packed_avx.c
+++ b/drivers/net/virtio/virtio_rxtx_packed_avx.c
@@ -328,9 +328,10 @@ virtio_vec_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 			 */
 			uint16_t csum = 0, off;
 
-			rte_raw_cksum_mbuf(m, hdr->csum_start,
+			if (rte_raw_cksum_mbuf(m, hdr->csum_start,
 				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
-				&csum);
+				&csum) < 0)
+				return -1;
 			if (likely(csum != 0xffff))
 				csum = ~csum;
 			off = hdr->csum_offset + hdr->csum_start;
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf

[Thomas Monjalon]

> rte_raw_cksum_mbuf can fail, so we should check to see if it
> has. If so, return with an error.
> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")

It is an old bug, so it should be backported, right?
Cc: stable@dpdk.org

> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
> 
>  drivers/net/virtio/virtio_rxtx.c            | 5 +++--
>  drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--

Please use --cc-cmd devtools/get-maintainer.sh
so the maintainers are aware.

Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf

[Maxime Coquelin]

On 10/1/20 12:22 PM, Chas Williams wrote:
> rte_raw_cksum_mbuf can fail, so we should check to see if it
> has. If so, return with an error.
> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> 
> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c            | 5 +++--
>  drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf

[Thomas Monjalon]

09/10/2020 09:14, Maxime Coquelin:
> 
> On 10/1/20 12:22 PM, Chas Williams wrote:
> > rte_raw_cksum_mbuf can fail, so we should check to see if it
> > has. If so, return with an error.
> > 
> > Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> > 
> > Signed-off-by: Chas Williams <3chas3@gmail.com>
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series applied, thanks