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

[Simon Horman <horms@kernel.org>]

On Mon, Mar 03, 2025 at 11:00:35AM +0100, 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.

Yes, quite.