Re: [Intel-wired-lan] [iwl-net v2 5/5] ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

["Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@linux.intel.com>]

On 3/3/2025 11:00 AM, Przemek Kitszel wrote:
> On 2/28/25 18:17, Simon Horman wrote:
>> On Tue, Feb 25, 2025 at 10:08:49AM +0100, Martyna Szapar-Mudlaw wrote:
>>> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>
>>> Fix using the untrusted value of proto->raw.pkt_len in function
>>> ice_vc_fdir_parse_raw() by verifying if it does not exceed the
>>> VIRTCHNL_MAX_SIZE_RAW_PACKET value.
>>>
>>> Fixes: 99f419df8a5c ("ice: enable FDIR filters from raw binary 
>>> patterns for VFs")
>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar- 
>>> mudlaw@linux.intel.com>
>>> ---
>>>   .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 25 +++++++++++++------
>>>   1 file changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/ 
>>> drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
>>> index 14e3f0f89c78..6250629ee8f9 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
>>> @@ -835,18 +835,27 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
>>>       u8 *pkt_buf, *msk_buf __free(kfree);
>>>       struct ice_parser_result rslt;
>>>       struct ice_pf *pf = vf->pf;
>>> +    u16 pkt_len, udp_port = 0;
>>>       struct ice_parser *psr;
>>>       int status = -ENOMEM;
>>>       struct ice_hw *hw;
>>> -    u16 udp_port = 0;
>>> -    pkt_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
>>> -    msk_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
>>> +    if (!proto->raw.pkt_len)
>>> +        return -EINVAL;
>>
>> Hi Martyna,
>>
>> It seems to me that the use of __free() above will result in
>> kfree(msk_buf) being called here. But msk_buf is not initialised at this
>> point.
>>
>> My suggest would be to drop the use of __free().
>> But if not, I think that in order to be safe it would be best to do this
>> (completely untested;
>>
>>     u8 *pkt_buf, *msk_buf __free(kfree) = NULL;
> 
> Oh yeah!, thank you Simon for catching that.
> 
> I would say "naked __free()" was harmful here.
> 

Thank you for suggestions, will send fixed v3