netdev - Re: Re:Re: [linux-nics] [PATCH]e100 in linux-3.18.0: some potential bugs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Sat, 20 Dec 2014 04:52:51 -0800
From:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:	白家驹 <baijiaju1990@....com>
Cc:	netdev <netdev@...r.kernel.org>,
	e1000-devel <e1000-devel@...ts.sourceforge.net>
Subject: Re: Re:Re: [linux-nics] [PATCH]e100 in linux-3.18.0: some potential
 bugs

On Sat, 2014-12-20 at 20:40 +0800, 白家驹 wrote:
> Thank for the reply!
>  
> For the first reply:
> I let pci_pool_create fail on purpose to simulate insufficient memory,
> and then run the driver in reality, but the driver crashes.
>  
> For the second reply:
> I admit you are right, and my code style need to be improved.
> 

Added in netdev and e1000-devel back onto the CC, since Jia-Ju Bai
removed them in his reply

> 
> At 2014-12-20 18:18:09,"Jeff Kirsher" <jeffrey.t.kirsher@...el.com> wrote:
> >On Sat, 2014-12-20 at 15:40 +0800, Jia-Ju Bai wrote:
> >> I have actually tested e100 driver on the real hardware(Intel 82559
> >> PCI
> >> Ethernet Controller), and find some bugs:
> >> The target file is drivers/net/ethernet/intel/e100.c, which is used to
> >> build
> >> e100.ko.
> >> 
> >> (1) The function pci_pool_create is called by e100_probe when
> >> initializing
> >> the ethernet card driver. But when pci_pool_create is failed, which
> >> means
> >> that it returns NULL to nic->cbs_pool, the system crash will happen.
> >> Because
> >> pci_pool_alloc (in e100_alloc_cbs in e100_up in e100_open) need to use
> >> nic->cbs_pool to allocate the resource, but it is NULL. I suggest that
> >> a
> >> check can be added in the code to detect whether pci_pool_create
> >> returns
> >> NULL.
> >> (2) In the normal process, netif_napi_add is called in e100_probe, but
> >> netif_napi_del is not called in e100_remove. However, many other
> >> ethernet
> >> card drivers call them in pairs, even in the error handling paths,
> >> such as
> >> r8169 and igb.
> >> 
> >> Meanwhile, I also write the patch to fix the bugs. I have run the
> >> patch on
> >> the hardware, it can work normally and fix the above bugs.
> >
> >Did you actually experience an issue?  Or is this a theoretical issue
> >that was never actually seen?
> >
> >> 
> >> diff --git a/drivers/net/ethernet/intel/e100.c
> >> b/drivers/net/ethernet/intel/e100.c
> >> index 781065e..2631d3f 100644
> >> --- a/drivers/net/ethernet/intel/e100.c
> >> +++ b/drivers/net/ethernet/intel/e100.c
> >> @@ -2969,6 +2969,11 @@ static int e100_probe(struct pci_dev *pdev,
> >> const
> >> struct pci_device_id *ent)
> >>                            nic->params.cbs.max * sizeof(struct cb),
> >>                            sizeof(u32),
> >>                            0);
> >> +       if(!(nic->cbs_pool))
> >> +       {
> >> +               err = -ENOMEM;
> >> +               goto err_out_pool;
> >> +       }
> >>         netif_info(nic, probe, nic->netdev,
> >
> >Minor nit-pick but your open bracket needs to be on the same line as the
> >if statement AND you need a space between the 'if' and ().  So the above
> >code should look like:
> >	if (!nic->cbs_pool) {
> >		err = -ENOMEM;
> >		goto err_out_pool;
> >	}
> 
> 



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)