Return-path: Received: from mail.atheros.com ([12.36.123.2]:44359 "EHLO mail.atheros.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755782Ab0F2Q0F convert rfc822-to-8bit (ORCPT ); Tue, 29 Jun 2010 12:26:05 -0400 From: Rajkumar Manoharan To: "Luis R. Rodriguez" CC: "linux-wireless@vger.kernel.org" , "linux-bluetooth@vger.kernel.org" Date: Tue, 29 Jun 2010 21:56:00 +0530 Subject: RE: [PATCH] compat: Fix panic caused by NULL pointer derefence in rtnl_fill_ifinfo Message-ID: <44EE5C37ADC36343B0625A05DD408C4850DAB4D6BD@CHEXMB-01.global.atheros.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: >________________________________________ >From: Luis R. Rodriguez [mcgrof@gmail.com] >Sent: Tuesday, June 29, 2010 8:51 PM >To: Rajkumar Manoharan >Cc: mcgrof@kernel.org; linux-wireless@vger.kernel.org; linux-bluetooth@vger.kernel.org >Subject: Re: [PATCH] compat: Fix panic caused by NULL pointer derefence in rtnl_fill_ifinfo >On Mon, Jun 28, 2010 at 11:38 PM, Rajkumar Manoharan wrote: > get stats netdev ops is blindy called for older kernels (< 2.6.29) and > so assigning a NULL pointer from netdev_attach_ops causes a NULL pointer > dereference. > > By default, netdev alloc provides an internal stats reference. So fill > this only if ndo_get_stats is defined. > > Signed-off-by: Rajkumar Manoharan > --- > compat/compat-2.6.29.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/compat/compat-2.6.29.c b/compat/compat-2.6.29.c > index f94aed8..2e7e623 100644 > --- a/compat/compat-2.6.29.c > +++ b/compat/compat-2.6.29.c > @@ -35,7 +35,8 @@ void netdev_attach_ops(struct net_device *dev, > dev->change_mtu = ops->ndo_change_mtu; > dev->set_mac_address = ops->ndo_set_mac_address; > dev->tx_timeout = ops->ndo_tx_timeout; > - dev->get_stats = ops->ndo_get_stats; > + if (ops->ndo_get_stats) > + dev->get_stats = ops->ndo_get_stats; > > If ops->ndo_get_stats is NULL then dev->get_stats will be set to NULL. > Do you know for sure this fixes something? If so can you explain how? > I used to have a macro that checked for not NULL and if true set the > callback but then later realized after Johannes poked me that this is > silly given that if the op is NULL you are just setting it to NULL. > > I don't see the potential crash here. > > Luis During alloc_netdev, get_stats is set to default callback (internal_stats). It won't be NULL. Based on this assumption, get_stats is invoked blindly in rtnl_fill_ifinfo without NULL check. So either get_stats set with default callback or callback assigned by module. It shouldn't be NULL. Rajkumar