Re: [PATCH v5 02/11] eal: add new secure free function

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 12 Feb 2025 10:01:13 +0800
fengchengwen <fengchengwen@huawei.com> wrote:

> On 2025/2/12 1:35, Stephen Hemminger wrote:
> > Although internally rte_free does poison the buffer in most
> > cases, it is useful to have function that explicitly does
> > this to avoid any security issues.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/eal/common/rte_malloc.c  | 30 ++++++++++++++++++++++++------
> >  lib/eal/include/rte_malloc.h | 18 ++++++++++++++++++
> >  lib/eal/version.map          |  3 +++
> >  3 files changed, 45 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/eal/common/rte_malloc.c b/lib/eal/common/rte_malloc.c
> > index 3eed4d4be6..c9e0f4724f 100644
> > --- a/lib/eal/common/rte_malloc.c
> > +++ b/lib/eal/common/rte_malloc.c
> > @@ -15,6 +15,7 @@
> >  #include <rte_eal_memconfig.h>
> >  #include <rte_common.h>
> >  #include <rte_spinlock.h>
> > +#include <rte_string_fns.h>
> >  
> >  #include <eal_trace_internal.h>
> >  
> > @@ -27,27 +28,44 @@
> >  
> >  
> >  /* Free the memory space back to heap */
> > -static void
> > -mem_free(void *addr, const bool trace_ena)
> > +static inline void
> > +mem_free(void *addr, const bool trace_ena, bool zero)
> >  {
> > +	struct malloc_elem *elem;
> > +
> >  	if (trace_ena)
> >  		rte_eal_trace_mem_free(addr);
> >  
> > -	if (addr == NULL) return;
> > -	if (malloc_heap_free(malloc_elem_from_data(addr)) < 0)
> > +	if (addr == NULL)
> > +		return;
> > +
> > +	elem = malloc_elem_from_data(addr);
> > +	if (zero) {
> > +		size_t data_len = elem->size - MALLOC_ELEM_OVERHEAD;  
> 
> this will make rte_malloc know the layout of malloc-elem.
> Prefer to add extra paramter, e.g. malloc_heap_free(elem, bool zero)

Don't understand, these are functions inside the rte_malloc implementation file.
The layout of malloc_elem is already known here and nothing visible in API or ABI is changing.
>   
> > +
> > +/**
> > + * Frees the memory space pointed to by the provided pointer
> > + * and guarantees it will be zero'd before reuse.
> > + *
> > + * This pointer must have been returned by a previous call to
> > + * rte_malloc(), rte_zmalloc(), rte_calloc() or rte_realloc(). The behaviour of
> > + * rte_free() is undefined if the pointer does not match this requirement.  
> 
> Suggest add notice: The value may be cleared twice, which affects the performance.

That could easily change with a little work, this is only for crypto keys
so performance doesn't matter.

> > + *
> > + * If the pointer is NULL, the function does nothing.
> > + *
> > + * @param ptr
> > + *   The pointer to memory to be freed.
> > + */
> > +__rte_experimental
> > +void
> > +rte_free_sensitive(void *ptr);  
> 
> one line is OK.
> void rte_free_sensitive(void *ptr);

Yes it could be on one line, and more compact is my preferred style.
But other functions in this file and DPDK style guide say it should be on its own line.