> The sum of offset + length, both 32 bit unsigned integers, could wrap
> around, causing comparisons to give the wrong result.
> This was fixed by using 64 bit instead of 32 bit for calculating the sum.
>
> Note:
> When the branch is not taken for the initial "if ((uint64_t)off + len >
> rte_pktmbuf_pkt_len(m))" comparison, the sum is known to not exceed the
> maximum possible value of rte_pktmbuf_pkt_len(m), UINT32_MAX, and
> following sum calculations can proceed using 32 bit.
>
> Also, fixed a related bug in an mbuf test case:
> It expected reading a length of UINT_MAX from a non-zero offset to not
> fail.
> And due to the offset+length wraparound bug, the read operation did not
> fail.
> This test case was updated to expect the read operation to fail.
>
> Fixes: b84110e7baa2 ("mbuf: add function to read packet data")
> Fixes: 7b295dceea07 ("test/mbuf: add unit test cases")
> Cc: [email protected]
>
> Signed-off-by: Morten Brørup <[email protected]>
> ---
> v2:
> * Fixed mbuf test case. (Marat Khalili)
> ---
> app/test/test_mbuf.c | 18 +++++++++---------
> lib/mbuf/rte_mbuf.c | 2 +-
> lib/mbuf/rte_mbuf.h | 2 +-
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index a41d2d0f97..db23259745 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -2037,13 +2037,13 @@ test_pktmbuf_read_from_offset(struct
> rte_mempool *pktmbuf_pool)
> /* read length greater than mbuf data_len */
> if (rte_pktmbuf_read(m, hdr_len, rte_pktmbuf_data_len(m) + 1,
> NULL) != NULL)
> - GOTO_FAIL("%s: Requested len is larger than mbuf data len!\n",
> + GOTO_FAIL("%s: Requested offset + len is larger than mbuf data
> len!\n",
> __func__);
>
> /* read length greater than mbuf pkt_len */
> if (rte_pktmbuf_read(m, hdr_len, rte_pktmbuf_pkt_len(m) + 1,
> NULL) != NULL)
> - GOTO_FAIL("%s: Requested len is larger than mbuf pkt len!\n",
> + GOTO_FAIL("%s: Requested offset + len is larger than mbuf pkt
> len!\n",
> __func__);
>
> /* read data of zero len from valid offset */
> @@ -2065,21 +2065,21 @@ test_pktmbuf_read_from_offset(struct
> rte_mempool *pktmbuf_pool)
>
> /* read data of max length from valid offset */
> data_copy = rte_pktmbuf_read(m, hdr_len, UINT_MAX, NULL);
> - if (data_copy == NULL)
> - GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> - /* check if the received address is the beginning of data segment */
> - if (data_copy != data)
> - GOTO_FAIL("%s: Corrupted data address!\n", __func__);
> + if (data_copy != NULL)
> + GOTO_FAIL("%s: Requested offset + max len is larger than mbuf
> pkt len!\n",
> + __func__);
>
> /* try to read from mbuf with max size offset */
> data_copy = rte_pktmbuf_read(m, UINT_MAX, 0, NULL);
> if (data_copy != NULL)
> - GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> + GOTO_FAIL("%s: Requested max offset is larger than mbuf pkt
> len!\n",
> + __func__);
>
> /* try to read from mbuf with max size offset and len */
> data_copy = rte_pktmbuf_read(m, UINT_MAX, UINT_MAX, NULL);
> if (data_copy != NULL)
> - GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> + GOTO_FAIL("%s: Requested max offset + max len is larger than
> mbuf pkt len!\n",
> + __func__);
>
> rte_pktmbuf_dump(stdout, m, rte_pktmbuf_pkt_len(m));
>
> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> index a5d16e4c97..c2476e7704 100644
> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -795,7 +795,7 @@ const void *__rte_pktmbuf_read(const struct rte_mbuf
> *m, uint32_t off,
> const struct rte_mbuf *seg = m;
> uint32_t buf_off = 0, copy_len;
>
> - if (off + len > rte_pktmbuf_pkt_len(m))
> + if ((uint64_t)off + len > rte_pktmbuf_pkt_len(m))
Thanks Morten for addressing it.
Just as a nit, for 32-bit systems, wouldn't it be more friendly:
If (off + len < off || off + len > rte_pktmbuf_pkt_len(m))
?
Same for comparison in rte_pktmbuf_read().
With or without suggested change:
Acked-by: Konstantin Ananyev <[email protected]>
> return NULL;
>
> while (off >= rte_pktmbuf_data_len(seg)) {
> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> index 592af2388c..d6602f74bc 100644
> --- a/lib/mbuf/rte_mbuf.h
> +++ b/lib/mbuf/rte_mbuf.h
> @@ -1843,7 +1843,7 @@ const void *__rte_pktmbuf_read(const struct rte_mbuf
> *m, uint32_t off,
> static inline const void *rte_pktmbuf_read(const struct rte_mbuf *m,
> uint32_t off, uint32_t len, void *buf)
> {
> - if (likely(off + len <= rte_pktmbuf_data_len(m)))
> + if (likely((uint64_t)off + len <= rte_pktmbuf_data_len(m)))
> return rte_pktmbuf_mtod_offset(m, char *, off);
> else
> return __rte_pktmbuf_read(m, off, len, buf);
> --
> 2.43.0
