| Message ID | 20260824132248.3396788-1-aleksandr.loktionov@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-net,v2] ice: fix bound parser hash offset before reading packet data | expand |
On Mon, Aug 24, 2026 at 03:22:48PM +0200, Aleksandr Loktionov wrote: > ice_rt_ho_set() uses the HO register as the starting offset of an > ICE_GPR_HV_SIZE-byte memcpy() out of rt->pkt_buf. Potentially HO can > be advanced by user-controlled data reachable through > ice_parse_raw_rss_pattern() -> ice_parser_run() -> > ice_parser_rt_execute() -> ice_rt_gpr_set() -> ice_rt_ho_set(), i.e. a > VF-supplied raw RSS pattern (virt/rss.c), with no bound against the > size of pkt_buf. > > Clamp HO to the last offset from which ICE_GPR_HV_SIZE bytes can still > be read out of pkt_buf, deriving the limit from sizeof(rt->pkt_buf) > so it stays correct if the packet buffer layout changes. > > ice_parser_rt_pktbuf_set() stores the caller's raw pkt_len in > rt->pkt_len, even though it only ever copies min(ICE_PARSER_MAX_PKT_LEN, > pkt_len) bytes into rt->pkt_buf. Both ice_parse_raw_rss_pattern() and > ice_vc_fdir_parse_raw() pass a VF-supplied pkt_len of up to > VIRTCHNL_MAX_SIZE_RAW_PACKET (1024), i.e. larger than > ICE_PARSER_MAX_PKT_LEN (504). With HO now capped at 504, the > "HO >= pkt_len" loop exit in ice_parser_rt_execute() would never be > reached for such an oversized pkt_len. Store the already-clamped > length instead, so rt->pkt_len always matches what was actually copied > into rt->pkt_buf and the loop-exit check remains a valid bound > regardless of the caller-supplied pkt_len. > > Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop") > Cc: stable@vger.kernel.org > Cc: horms@kernel.org > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > --- > v1 -> v2: > - also clamp rt->pkt_len in ice_parser_rt_pktbuf_set() to the same > ICE_PARSER_MAX_PKT_LEN bound already used for the rt->pkt_buf copy, > so the "HO >= pkt_len" loop-exit in ice_parser_rt_execute() can't be > bypassed by an oversized pkt_len from the raw RSS/FDIR VF paths > (reported in review) > - dropped Przemek's Reviewed-by since the patch changed Thanks for the update. Reviewed-by: Simon Horman <horms@kernel.org>
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: Thursday, September 3, 2026 1:12 PM > To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org > Subject: Re: [PATCH iwl-net v2] ice: fix bound parser hash offset before reading > packet data > > On Mon, Aug 24, 2026 at 03:22:48PM +0200, Aleksandr Loktionov wrote: > > ice_rt_ho_set() uses the HO register as the starting offset of an > > ICE_GPR_HV_SIZE-byte memcpy() out of rt->pkt_buf. Potentially HO can > > be advanced by user-controlled data reachable through > > ice_parse_raw_rss_pattern() -> ice_parser_run() -> > > ice_parser_rt_execute() -> ice_rt_gpr_set() -> ice_rt_ho_set(), i.e. a > > VF-supplied raw RSS pattern (virt/rss.c), with no bound against the > > size of pkt_buf. > > > > Clamp HO to the last offset from which ICE_GPR_HV_SIZE bytes can still > > be read out of pkt_buf, deriving the limit from sizeof(rt->pkt_buf) so > > it stays correct if the packet buffer layout changes. > > > > ice_parser_rt_pktbuf_set() stores the caller's raw pkt_len in > > rt->pkt_len, even though it only ever copies > > rt->min(ICE_PARSER_MAX_PKT_LEN, > > pkt_len) bytes into rt->pkt_buf. Both ice_parse_raw_rss_pattern() and > > ice_vc_fdir_parse_raw() pass a VF-supplied pkt_len of up to > > VIRTCHNL_MAX_SIZE_RAW_PACKET (1024), i.e. larger than > > ICE_PARSER_MAX_PKT_LEN (504). With HO now capped at 504, the "HO >= > > pkt_len" loop exit in ice_parser_rt_execute() would never be reached > > for such an oversized pkt_len. Store the already-clamped length > > instead, so rt->pkt_len always matches what was actually copied into > > rt->pkt_buf and the loop-exit check remains a valid bound regardless > > of the caller-supplied pkt_len. > > > > Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop") > > Cc: stable@vger.kernel.org > > Cc: horms@kernel.org > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > > --- > > v1 -> v2: > > - also clamp rt->pkt_len in ice_parser_rt_pktbuf_set() to the same > > ICE_PARSER_MAX_PKT_LEN bound already used for the rt->pkt_buf copy, > > so the "HO >= pkt_len" loop-exit in ice_parser_rt_execute() can't be > > bypassed by an oversized pkt_len from the raw RSS/FDIR VF paths > > (reported in review) > > - dropped Przemek's Reviewed-by since the patch changed > > Thanks for the update. > > Reviewed-by: Simon Horman <horms@kernel.org> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
diff --git a/drivers/net/ethernet/intel/ice/ice_parser_rt.c b/drivers/net/ethernet/intel/ice/ice_parser_rt.c index 3995d66..b330005 100644 --- a/drivers/net/ethernet/intel/ice/ice_parser_rt.c +++ b/drivers/net/ethernet/intel/ice/ice_parser_rt.c @@ -10,6 +10,8 @@ static void ice_rt_tsr_set(struct ice_parser_rt *rt, u16 tsr) static void ice_rt_ho_set(struct ice_parser_rt *rt, u16 ho) { + /* keep the ICE_GPR_HV_SIZE-byte read below within pkt_buf */ + ho = min_t(u16, ho, sizeof(rt->pkt_buf) - ICE_GPR_HV_SIZE); rt->gpr[ICE_GPR_HO_IDX] = ho; memcpy(&rt->gpr[ICE_GPR_HV_IDX], &rt->pkt_buf[ho], ICE_GPR_HV_SIZE); } @@ -106,7 +108,7 @@ void ice_parser_rt_pktbuf_set(struct ice_parser_rt *rt, const u8 *pkt_buf, u16 ho = rt->gpr[ICE_GPR_HO_IDX]; memcpy(rt->pkt_buf, pkt_buf, len); - rt->pkt_len = pkt_len; + rt->pkt_len = len; memcpy(&rt->gpr[ICE_GPR_HV_IDX], &rt->pkt_buf[ho], ICE_GPR_HV_SIZE); }
ice_rt_ho_set() uses the HO register as the starting offset of an ICE_GPR_HV_SIZE-byte memcpy() out of rt->pkt_buf. Potentially HO can be advanced by user-controlled data reachable through ice_parse_raw_rss_pattern() -> ice_parser_run() -> ice_parser_rt_execute() -> ice_rt_gpr_set() -> ice_rt_ho_set(), i.e. a VF-supplied raw RSS pattern (virt/rss.c), with no bound against the size of pkt_buf. Clamp HO to the last offset from which ICE_GPR_HV_SIZE bytes can still be read out of pkt_buf, deriving the limit from sizeof(rt->pkt_buf) so it stays correct if the packet buffer layout changes. ice_parser_rt_pktbuf_set() stores the caller's raw pkt_len in rt->pkt_len, even though it only ever copies min(ICE_PARSER_MAX_PKT_LEN, pkt_len) bytes into rt->pkt_buf. Both ice_parse_raw_rss_pattern() and ice_vc_fdir_parse_raw() pass a VF-supplied pkt_len of up to VIRTCHNL_MAX_SIZE_RAW_PACKET (1024), i.e. larger than ICE_PARSER_MAX_PKT_LEN (504). With HO now capped at 504, the "HO >= pkt_len" loop exit in ice_parser_rt_execute() would never be reached for such an oversized pkt_len. Store the already-clamped length instead, so rt->pkt_len always matches what was actually copied into rt->pkt_buf and the loop-exit check remains a valid bound regardless of the caller-supplied pkt_len. Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop") Cc: stable@vger.kernel.org Cc: horms@kernel.org Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> --- v1 -> v2: - also clamp rt->pkt_len in ice_parser_rt_pktbuf_set() to the same ICE_PARSER_MAX_PKT_LEN bound already used for the rt->pkt_buf copy, so the "HO >= pkt_len" loop-exit in ice_parser_rt_execute() can't be bypassed by an oversized pkt_len from the raw RSS/FDIR VF paths (reported in review) - dropped Przemek's Reviewed-by since the patch changed --- drivers/net/ethernet/intel/ice/ice_parser_rt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)