Re: [PATCH] a100u2w: Added sanitization for pointer dereference using a value from hardware. Detected using Carburizer (http://lwn.net/Articles/479653/)

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2012-12-27 at 02:59 -0600, Asim Kadav wrote:
> Signed-off-by: Asim Kadav <kadav@cs.wisc.edu>
> ---
>  drivers/scsi/a100u2w.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/a100u2w.c b/drivers/scsi/a100u2w.c
> index 0163457..c2ca15c 100644
> --- a/drivers/scsi/a100u2w.c
> +++ b/drivers/scsi/a100u2w.c
> @@ -821,6 +821,9 @@ static irqreturn_t orc_interrupt(struct orc_host * host)
>  		/* Get the SCB index of the SCB to service */
>  		scb_index = inb(host->base + ORC_RQUEUE);
>  
> +		/* Sanitize incoming index */
> +		scb_index &= 31;
> +

If your theory is that the hardware just returned a bogus value, this
isn't the right way to sanitise it because the chances are you'll
complete the wrong command and cause corruption: you'd have to halt the
entire system at that point.  Also, I don't understand why you think the
value should only be 0-31?  The size of variable allocated there is for
SCBs up to 243, no idea why, since some of the allocation routines will
search up to 256.  However, safety from overrun should be guaranteed at
least at the system level by the can_queue value.

Double checking hardware values isn't something we habitually do unless
there's a known reason for it (like the state machine does throw bogus
values with a defined recovery procedure).  We definitely don't run in
the mode where you can't trust your hardware.

James