| Message ID | 20221018092526.4035344-1-keescook@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| 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
On 10/18/2022 5:20 AM, Ruhl, Michael J wrote: >> -----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; > Fix seems straight forward. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> This feels like something we could tag with a Fixes and send through net? Thanks, Jake
> -----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(-)