On Tue, 17 May 2022 17:57:23 +0800 Yuanjun Gong ruc_gongyuanjun@163.com wrote:

From: Gong Yuanjun ruc_gongyuanjun@163.com

In hist_trigger_elt_data_alloc(), elt_data is freed by hist_elt_data_free() if kcalloc fails.

static int hist_trigger_elt_data_alloc(struct tracing_map_elt *elt) { ... elt_data->field_var_str = kcalloc(n_str, sizeof(char *), GFP_KERNEL); if (!elt_data->field_var_str) {

But at this position elt_data->n_field_var_str is zero.

            hist_elt_data_free(elt_data);
            return -EINVAL;
    }

...}

In hist_elt_data_free() the elt_data->field_var_str field should be checked before dereference.

Signed-off-by: Gong Yuanjun ruc_gongyuanjun@163.com

kernel/trace/trace_events_hist.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 44db5ba9cabb..73177c9f94b2 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1576,11 +1576,11 @@ static void hist_elt_data_free(struct hist_elt_data *elt_data) { unsigned int i;

• for (i = 0; i < elt_data->n_field_var_str; i++)

This loop will not execute because n_field_var_str is zero, and 0 < 0 is false.

• kfree(elt_data->field_var_str[i]);
  

• kfree(elt_data->field_var_str);

freeing a NULL pointer is OK to do.

There is nothing wrong with the current code. NACK.

-- Steve

• if (elt_data->field_var_str) {

• for (i = 0; i < elt_data->n_field_var_str; i++)
  

• 	kfree(elt_data->field_var_str[i]);
  

• kfree(elt_data->field_var_str);
  

• } kfree(elt_data->comm); kfree(elt_data);

}