Not enough memory be allocated for mempool which cause out of bound memory access when get its private member. Fixes: 923ceaeac140 ("test/mempool: add unit test cases") Cc: stable@dpdk.org Signed-off-by: Wenwu Ma <wenwux.ma@intel.com> --- v1: * Refine commit log. app/test/test_mempool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c index 084842fda..fc06a9c6f 100644 --- a/app/test/test_mempool.c +++ b/app/test/test_mempool.c @@ -543,7 +543,8 @@ test_mempool(void) mp_stack_mempool_iter = rte_mempool_create("test_iter_obj", MEMPOOL_SIZE, MEMPOOL_ELT_SIZE, - RTE_MEMPOOL_CACHE_MAX_SIZE, 0, + RTE_MEMPOOL_CACHE_MAX_SIZE, + sizeof(struct rte_pktmbuf_pool_private), NULL, NULL, my_obj_init, NULL, SOCKET_ID_ANY, 0); -- 2.25.1
Not enough memory be allocated for mempool which cause out of bound memory access when get its private member. Fixes: 923ceaeac140 ("test/mempool: add unit test cases") Cc: stable@dpdk.org Signed-off-by: Wenwu Ma <wenwux.ma@intel.com> --- v1: * Refine commit log. app/test/test_mempool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c index 084842fda..fc06a9c6f 100644 --- a/app/test/test_mempool.c +++ b/app/test/test_mempool.c @@ -543,7 +543,8 @@ test_mempool(void) mp_stack_mempool_iter = rte_mempool_create("test_iter_obj", MEMPOOL_SIZE, MEMPOOL_ELT_SIZE, - RTE_MEMPOOL_CACHE_MAX_SIZE, 0, + RTE_MEMPOOL_CACHE_MAX_SIZE, + sizeof(struct rte_pktmbuf_pool_private), NULL, NULL, my_obj_init, NULL, SOCKET_ID_ANY, 0); -- 2.25.1
13/04/2021 22:05, Wenwu Ma: > Amount of allocated memory was not enough for mempool > which cause buffer overflow when access fields of mempool > private structure in the rte_pktmbuf_priv_size function. Was it causing the test to fail? How do you reproduce the overflow? > Fixes: 923ceaeac140 ("test/mempool: add unit test cases") > Cc: stable@dpdk.org > > Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
Amount of allocated memory was not enough for mempool which cause buffer overflow when access fields of mempool private structure in the rte_pktmbuf_priv_size function. Fixes: 923ceaeac140 ("test/mempool: add unit test cases") Cc: stable@dpdk.org Signed-off-by: Wenwu Ma <wenwux.ma@intel.com> --- v2: - refine commit log. --- app/test/test_mempool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c index 084842fda..fc06a9c6f 100644 --- a/app/test/test_mempool.c +++ b/app/test/test_mempool.c @@ -543,7 +543,8 @@ test_mempool(void) mp_stack_mempool_iter = rte_mempool_create("test_iter_obj", MEMPOOL_SIZE, MEMPOOL_ELT_SIZE, - RTE_MEMPOOL_CACHE_MAX_SIZE, 0, + RTE_MEMPOOL_CACHE_MAX_SIZE, + sizeof(struct rte_pktmbuf_pool_private), NULL, NULL, my_obj_init, NULL, SOCKET_ID_ANY, 0); -- 2.25.1
On Tue, Apr 13, 2021 at 01:52:26PM +0200, Thomas Monjalon wrote:
> 13/04/2021 22:05, Wenwu Ma:
> > Amount of allocated memory was not enough for mempool
> > which cause buffer overflow when access fields of mempool
> > private structure in the rte_pktmbuf_priv_size function.
>
> Was it causing the test to fail?
> How do you reproduce the overflow?
In the test, right after the rte_mempool_create(), the function
rte_mempool_obj_iter() is called too initialize the mempool objects with
the rte_pktmbuf_init() callback function. This callback expects that the
mempool is a packet pool, i.e. its private area is a struct
rte_pktmbuf_pool_private structure.
In the current test, the size of the private area is 0, which probably
causes the function rte_pktmbuf_priv_size() to return an unpredictable
value, and this value is used as a size in a memset.
This part of the test was added in commit 923ceaeac140 ("test/mempool:
add unit test cases").
Instead of changing the size of the private area like done in the patch,
I suggest to use another callback than rte_pktmbuf_init(). After all,
this is a mempool test, so we should not rely on mbuf features. The
function my_obj_init() could be used like in other places of the test,
like this:
@@ -552,7 +552,7 @@ test_mempool(void)
GOTO_ERR(ret, err);
/* test to initialize mempool objects and memory */
- nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, rte_pktmbuf_init,
+ nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, my_obj_init,
NULL);
if (nb_objs == 0)
GOTO_ERR(ret, err);
Wenwu, does it solve your issue?
Regards,
Olivier
Olivier Matz <olivier.matz@6wind.com> writes: > On Tue, Apr 13, 2021 at 01:52:26PM +0200, Thomas Monjalon wrote: >> 13/04/2021 22:05, Wenwu Ma: >> > Amount of allocated memory was not enough for mempool >> > which cause buffer overflow when access fields of mempool >> > private structure in the rte_pktmbuf_priv_size function. >> >> Was it causing the test to fail? >> How do you reproduce the overflow? > > In the test, right after the rte_mempool_create(), the function > rte_mempool_obj_iter() is called too initialize the mempool objects with > the rte_pktmbuf_init() callback function. This callback expects that the > mempool is a packet pool, i.e. its private area is a struct > rte_pktmbuf_pool_private structure. > > In the current test, the size of the private area is 0, which probably > causes the function rte_pktmbuf_priv_size() to return an unpredictable > value, and this value is used as a size in a memset. Is it possible to have rte_mempool_get_priv() detect that the private area isn't valid and return a ref to a const static member for this that will have the correct mbuf_priv_size? There isn't really documentation that I can find that describes this corner case with the mempool private data section. Actually, it doesn't really say what happens if private data size is 0, so maybe a documentation update should go with this test case fix, too? > This part of the test was added in commit 923ceaeac140 ("test/mempool: > add unit test cases"). > > Instead of changing the size of the private area like done in the patch, > I suggest to use another callback than rte_pktmbuf_init(). After all, > this is a mempool test, so we should not rely on mbuf features. The > function my_obj_init() could be used like in other places of the test, > like this: > > @@ -552,7 +552,7 @@ test_mempool(void) > GOTO_ERR(ret, err); > > /* test to initialize mempool objects and memory */ > - nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, rte_pktmbuf_init, > + nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, my_obj_init, > NULL); > if (nb_objs == 0) > GOTO_ERR(ret, err); > > > Wenwu, does it solve your issue? > > > Regards, > Olivier
On Thu, Apr 15, 2021 at 08:51:27AM -0400, Aaron Conole wrote: > Olivier Matz <olivier.matz@6wind.com> writes: > > > On Tue, Apr 13, 2021 at 01:52:26PM +0200, Thomas Monjalon wrote: > >> 13/04/2021 22:05, Wenwu Ma: > >> > Amount of allocated memory was not enough for mempool > >> > which cause buffer overflow when access fields of mempool > >> > private structure in the rte_pktmbuf_priv_size function. > >> > >> Was it causing the test to fail? > >> How do you reproduce the overflow? > > > > In the test, right after the rte_mempool_create(), the function > > rte_mempool_obj_iter() is called too initialize the mempool objects with > > the rte_pktmbuf_init() callback function. This callback expects that the > > mempool is a packet pool, i.e. its private area is a struct > > rte_pktmbuf_pool_private structure. > > > > In the current test, the size of the private area is 0, which probably > > causes the function rte_pktmbuf_priv_size() to return an unpredictable > > value, and this value is used as a size in a memset. > > Is it possible to have rte_mempool_get_priv() detect that the private > area isn't valid and return a ref to a const static member for this that > will have the correct mbuf_priv_size? There isn't really documentation > that I can find that describes this corner case with the mempool private > data section. Actually, it doesn't really say what happens if private > data size is 0, so maybe a documentation update should go with this test > case fix, too? Good point, we can indeed add something in the API documentation. To detect that the private area is not big enough in rte_pktmbuf_init(), unfortunatly the function has no return code, but for now we can add at least an RTE_ASSERT() (only active when -DRTE_ENABLE_ASSERT is passed), as it's already done for other checks. I can do a new version of the patch. Wenwu, is it ok for you? In a second step, we can think about changing the API of all mempool callbacks and their wrappers to add a return code. > > This part of the test was added in commit 923ceaeac140 ("test/mempool: > > add unit test cases"). > > > > Instead of changing the size of the private area like done in the patch, > > I suggest to use another callback than rte_pktmbuf_init(). After all, > > this is a mempool test, so we should not rely on mbuf features. The > > function my_obj_init() could be used like in other places of the test, > > like this: > > > > @@ -552,7 +552,7 @@ test_mempool(void) > > GOTO_ERR(ret, err); > > > > /* test to initialize mempool objects and memory */ > > - nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, rte_pktmbuf_init, > > + nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, my_obj_init, > > NULL); > > if (nb_objs == 0) > > GOTO_ERR(ret, err); > > > > > > Wenwu, does it solve your issue? > > > > > > Regards, > > Olivier >
The function rte_pktmbuf_init() expects that the mempool private area is large enough and was previously initialized by rte_pktmbuf_pool_init(), which is not the case. This causes the function rte_pktmbuf_priv_size() to return an unpredictable value, and this value is used as a size in a memset. Replace the mempool object initializer by my_obj_init(), which does not have this constraint, and fits the needs for this test. Fixes: 923ceaeac140 ("test/mempool: add unit test cases") Cc: stable@dpdk.org Signed-off-by: Wenwu Ma <wenwux.ma@intel.com> --- app/test/test_mempool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c index 084842fdaa..3adadd6731 100644 --- a/app/test/test_mempool.c +++ b/app/test/test_mempool.c @@ -552,7 +552,7 @@ test_mempool(void) GOTO_ERR(ret, err); /* test to initialize mempool objects and memory */ - nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, rte_pktmbuf_init, + nb_objs = rte_mempool_obj_iter(mp_stack_mempool_iter, my_obj_init, NULL); if (nb_objs == 0) GOTO_ERR(ret, err); -- 2.29.2
27/04/2021 15:56, Olivier Matz:
> The function rte_pktmbuf_init() expects that the mempool private area is
> large enough and was previously initialized by rte_pktmbuf_pool_init(),
> which is not the case.
>
> This causes the function rte_pktmbuf_priv_size() to return an
> unpredictable value, and this value is used as a size in a memset.
>
> Replace the mempool object initializer by my_obj_init(), which does not
> have this constraint, and fits the needs for this test.
>
> Fixes: 923ceaeac140 ("test/mempool: add unit test cases")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
Replaced with Reported-by and added Olivier's signature
to match patch authorship.
Series applied, thanks.