| Message ID | 20221018092526.4035344-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igb: Proactively round up to kmalloc bucket size | expand |
>-----Original Message----- >From: Kees Cook <keescook@chromium.org> >Sent: Tuesday, October 18, 2022 5:25 AM >To: Ruhl@www.outflux.net; Ruhl, Michael J <michael.j.ruhl@intel.com> >Cc: Kees Cook <keescook@chromium.org>; Brandeburg, Jesse ><jesse.brandeburg@intel.com>; Nguyen, Anthony L ><anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; >Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; >Paolo Abeni <pabeni@redhat.com>; intel-wired-lan@lists.osuosl.org; >netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux- >hardening@vger.kernel.org >Subject: [PATCH v3 1/2] igb: Do not free q_vector unless new one was >allocated > >Avoid potential use-after-free condition under memory pressure. If the >kzalloc() fails, q_vector will be freed but left in the original >adapter->q_vector[v_idx] array position. > >Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> >Cc: Tony Nguyen <anthony.l.nguyen@intel.com> >Cc: "David S. Miller" <davem@davemloft.net> >Cc: Eric Dumazet <edumazet@google.com> >Cc: Jakub Kicinski <kuba@kernel.org> >Cc: Paolo Abeni <pabeni@redhat.com> >Cc: intel-wired-lan@lists.osuosl.org >Cc: netdev@vger.kernel.org >Signed-off-by: Kees Cook <keescook@chromium.org> >--- > drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >b/drivers/net/ethernet/intel/igb/igb_main.c >index f8e32833226c..6256855d0f62 100644 >--- a/drivers/net/ethernet/intel/igb/igb_main.c >+++ b/drivers/net/ethernet/intel/igb/igb_main.c >@@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter >*adapter, > if (!q_vector) { > q_vector = kzalloc(size, GFP_KERNEL); > } else if (size > ksize(q_vector)) { >- kfree_rcu(q_vector, rcu); >- q_vector = kzalloc(size, GFP_KERNEL); >+ struct igb_q_vector *new_q_vector; >+ >+ new_q_vector = kzalloc(size, GFP_KERNEL); >+ if (new_q_vector) >+ kfree_rcu(q_vector, rcu); >+ q_vector = new_q_vector; Ok, that makes more sense to me. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Mike > } else { > memset(q_vector, 0, size); > } >-- >2.34.1
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Kees Cook > Sent: Tuesday, October 18, 2022 2:55 PM > To: Ruhl@www.outflux.net; Ruhl, Michael J <michael.j.ruhl@intel.com> > Cc: Kees Cook <keescook@chromium.org>; intel-wired-lan@lists.osuosl.org; > linux-kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; > linux-hardening@vger.kernel.org; netdev@vger.kernel.org; Jakub Kicinski > <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller > <davem@davemloft.net> > Subject: [Intel-wired-lan] [PATCH v3 1/2] igb: Do not free q_vector unless > new one was allocated > > Avoid potential use-after-free condition under memory pressure. If the > kzalloc() fails, q_vector will be freed but left in the original > adapter->q_vector[v_idx] array position. > > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: Tony Nguyen <anthony.l.nguyen@intel.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index f8e32833226c..6256855d0f62 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter, if (!q_vector) { q_vector = kzalloc(size, GFP_KERNEL); } else if (size > ksize(q_vector)) { - kfree_rcu(q_vector, rcu); - q_vector = kzalloc(size, GFP_KERNEL); + struct igb_q_vector *new_q_vector; + + new_q_vector = kzalloc(size, GFP_KERNEL); + if (new_q_vector) + kfree_rcu(q_vector, rcu); + q_vector = new_q_vector; } else { memset(q_vector, 0, size); }
Avoid potential use-after-free condition under memory pressure. If the kzalloc() fails, q_vector will be freed but left in the original adapter->q_vector[v_idx] array position. Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> Cc: Tony Nguyen <anthony.l.nguyen@intel.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)