linux-kernel - Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation unnoticed

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1208993783.9245.10.camel@concordia.ozlabs.ibm.com>
Date:	Thu, 24 Apr 2008 09:36:23 +1000
From:	Michael Ellerman <michael@...erman.id.au>
To:	Roel Kluin <12o3l@...cali.nl>
Cc:	Segher Boessenkool <segher@...nel.crashing.org>,
	linuxppc-dev@...abs.org, paulus@...ba.org,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2 v2] mpic_u3msi: mpic_u3msi: failed allocation
	unnoticed

On Thu, 2008-04-24 at 00:25 +0200, Roel Kluin wrote:
> Segher Boessenkool wrote:
> >> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may
> >> return -ENOMEM, but hwirq of type irq_hw_number_t which is unsigned.
> > 
> >>      list_for_each_entry(entry, &pdev->msi_list, list) {
> >>          hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> >> -        if (hwirq < 0) {
> >> +        if (hwirq == -ENOMEM) {
> 
> > 
> > Please test for _all_ error values, instead.
> >
> > Segher
> 
> In this case -ENOMEM was _all_ error values, but I get your point.
> ---
> bitmap_find_free_region(), called by mpic_msi_alloc_hwirqs() may return
> signed, but hwirq is unsigned. A failed allocation remains unnoticed.
> 
> diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
> index 1d5a408..e790f39 100644
> --- a/arch/powerpc/sysdev/mpic_u3msi.c
> +++ b/arch/powerpc/sysdev/mpic_u3msi.c
> @@ -115,14 +115,16 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
>  	struct msi_desc *entry;
>  	struct msi_msg msg;
>  	u64 addr;
> +	int ret;
>  
>  	addr = find_ht_magic_addr(pdev);
>  	msg.address_lo = addr & 0xFFFFFFFF;
>  	msg.address_hi = addr >> 32;
>  
>  	list_for_each_entry(entry, &pdev->msi_list, list) {
> -		hwirq = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> -		if (hwirq < 0) {
> +		ret = mpic_msi_alloc_hwirqs(msi_mpic, 1);
> +		hwirq = ret;
> +		if (ret < 0) {
>  			pr_debug("u3msi: failed allocating hwirq\n");
>  			return hwirq;
>  		}

I'm not sure I like this.

I think the real bug is that we're using irq_hw_number_t to represent
something which isn't. At the end of the day we have to stash the hwirq
into the MSI message data, which is a u32.

I guess we could imagine a driver that does something magic to allow it
to put something bigger than a u32 in the MSI message, but I doubt it.

So I think mpic_msi_alloc_hwirqs() should return a long, which allows
it to return a full u32 plus the negative error values.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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