Re: [EXT] Re: [PATCH v1 1/2] ethdev: fix null pointer dereference

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/28/2023 3:40 PM, Ankur Dwivedi wrote:
>> ----------------------------------------------------------------------
>> On 2/23/2023 12:30 PM, Ankur Dwivedi wrote:
>>> The speed_fec_capa pointer can be null. So dereferencing the pointer
>>> is removed and only the pointer is captured in trace function.
>>> Fixed few more trace functions in which null pointer can be dereferenced.
>>>
>>> Coverity issue: 383238
>>> Bugzilla ID: 1162
>>> Fixes: 6679cf21d608 ("ethdev: add trace points")
>>> Fixes: ed04fd4072e9 ("ethdev: add trace points for flow")
>>>
>>
>> In below changes, pointers can be NULL at runtime, so agree on to the change,
>> with minor comments please see below.
>>
>>
>> But I don't think this is a fix for Bug 1162, which is an ASan reported error, can
>> you please drop that tag unless it is verified.
> The asan error reported in 1162 was because of capturing rte_trace_point_emit_string(parent->bus_info); in
> rte_eth_trace_find_next_of. I could not find the exact reason for the asan error with parent->bus_info. 
> 

Yeah, I also looked at it but not able to identify why it is complaining.

> But I think parent pointer can be NULL in rte_eth_find_next_of, so I removed the pointer reference. This resolved the asan error in 1162 as a side effect.
> 

Is it confirmed that patch fixing asan issue, I have not seen it in the
bugzilla comments. If it is confirmed, OK to keep Bugzilla ID tag.

And aside from the asan issue, OK to have this patch, because of reason
you described.

> -	rte_trace_point_emit_string(parent->name);
> -	rte_trace_point_emit_string(parent->bus_info);
> -	rte_trace_point_emit_int(parent->numa_node);
> +	rte_trace_point_emit_ptr(parent);
> 
> I will check if I can add an if condition to check if a pointer is NULL inside the trace function. If that works I will update this patch.
>>
>> <...>
>>
>>> @@ -2308,8 +2300,7 @@ RTE_TRACE_POINT_FP(
>>>  		int ret),
>>>  	rte_trace_point_emit_u16(port_id);
>>>  	rte_trace_point_emit_ptr(flow);
>>> -	rte_trace_point_emit_int(action->type);
>>> -	rte_trace_point_emit_ptr(action->conf);
>>> +	rte_trace_point_emit_ptr(action);
>>>  	rte_trace_point_emit_ptr(data);
>>>  	rte_trace_point_emit_int(ret);
>>
>> I think 'rte_flow_trace_create()' is missed, rest looks OK.
>>
>> Can you please double check 'rte_flow_trace_create()' too?
> Yes. Will add rte_flow_trace_create.
>>
>>>  )
>>> @@ -2349,14 +2340,8 @@ RTE_TRACE_POINT_FP(
>>>  		const struct rte_flow_indir_action_conf *conf,
>>>  		const struct rte_flow_action *action,
>>>  		const struct rte_flow_action_handle *handle),
>>> -	uint8_t ingress = conf->ingress;
>>> -	uint8_t egress = conf->egress;
>>> -	uint8_t transfer = conf->transfer;
>>> -
>>>  	rte_trace_point_emit_u16(port_id);
>>> -	rte_trace_point_emit_u8(ingress);
>>> -	rte_trace_point_emit_u8(egress);
>>> -	rte_trace_point_emit_u8(transfer);
>>> +	rte_trace_point_emit_ptr(conf);
>>>  	rte_trace_point_emit_ptr(action);
>>>  	rte_trace_point_emit_ptr(handle);
>>>  )
>>
>> This change is different than others, this is removing bitfield related local
>> variable assignment.
>>
>> According to bug 1167 that is causing a crash. So we need a separate patch to
>> either remove or fix bitfield related issues, for now I am OK to remove (as
>> done above).
>>
>> But can you please make another patch for bitfield issue and move above
>> change to that patch?
> 
> Yes, will move this change to fix bitfield patch.
>>
>> Thanks,
>> ferruh
>