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

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

[Asim Kadav]

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;
+
 		/* Translate it back to a host pointer */
 		scb = (struct orc_scb *) ((unsigned long) host->scb_virt + (unsigned long) (sizeof(struct orc_scb) * scb_index));
 		scb->status = 0x0;
-- 
1.7.11.3

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

[James Bottomley]

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

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

[Asim Kadav]

> 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.

I agree. If the hardware returns a bogus value, it immediately would crash.
Would a more appropriate patch be checking within the ORC_MAXQUEUE 
range and flagging an error?

> 
> 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.
> 

Hardware can fail for multiple reasons. Furthermore, such bugs are security
vulnerabilities too in the case of virtual hardware or USB drivers where
such values can be faked. The article in my patch gives more details.
http://lwn.net/Articles/479653/

Thanks,
Asim

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

[James Bottomley]

On Thu, 2012-12-27 at 08:12 -0600, Asim Kadav wrote:
> > 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.
> 
> I agree. If the hardware returns a bogus value, it immediately would crash.
> Would a more appropriate patch be checking within the ORC_MAXQUEUE 
> range and flagging an error?

It's probably really not worth it.  The u100w2 is a pretty old SCSI
driver.  I can't imagine there's more than a handful of them left.  As I
said, there's no evidence of a problem.

> > 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.
> > 
> 
> Hardware can fail for multiple reasons. Furthermore, such bugs are security
> vulnerabilities too in the case of virtual hardware or USB drivers where
> such values can be faked. The article in my patch gives more details.
> http://lwn.net/Articles/479653/

USB possibly, but this isn't a USB driver.  Virtual hardware is a bit
unlikely, since it's created by the host for the guest.  I don't believe
there's any hypervisor system that can survive attempted subversion by
the *host* (host protecting against guest, yes, but guest trying to
protect against host is a very different ball game).

James

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

[Asim Kadav]

> It's probably really not worth it.  The u100w2 is a pretty old SCSI
> driver.  I can't imagine there's more than a handful of them left.  As I
> said, there's no evidence of a problem.

I would argue that having hardened code is always better if you plan to
support this code at all. And these bugs manifest  more frequently as hardware 
gets older. Sanitizing inputs from hardware is a good idea and this one is 
particularly insidious since it uses it uses hardware values in pointer operations.

-Asim