RE: Questionable code in pci_sal_read

On Tue, 2005-01-25 at 12:47 -0800, Luck, Tony wrote:
> >Ah, yes, that looks wrong.  Looks like the check for (seg > 255) came
> >from the original pci_sal_read().  The original pci_sal_ext_read() did
> >check for (seg > 65535).  My bad.
> >
> >Thanks for catching this.
> 
> 
> So you (and Matthew Wilcox) are advocating this change?
> 
> ===== arch/ia64/pci/pci.c 1.66 vs edited =====
> --- 1.66/arch/ia64/pci/pci.c	2005-01-22 14:42:51 -08:00
> +++ edited/arch/ia64/pci/pci.c	2005-01-25 12:42:49 -08:00
> @@ -71,7 +71,7 @@
>  	u64 addr, mode, data = 0;
>  	int result = 0;
>  
> -	if ((seg > 255) || (bus > 255) || (devfn > 255) || (reg > 4095))
> +	if ((seg > 65535) || (bus > 255) || (devfn > 255) || (reg > 4095))
>  		return -EINVAL;
>  
>  	if ((seg | reg) <= 255) {
> 
> "seg", "bus", etc. are all "int" ... should we be extra paranoid
> and check for negative values (or change the definitions to unsigned),
> or is that over the top?

We should definitely change them to unsigned; it's a real problem
that has bitten us already.  In fact, I wonder if Andreas was
looking at this code as a result of the bug I opened yesterday ;-)

I'm testing a patch right now, and it includes the "seg > 65535"
change as well.

-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html