[PATCH 6.1 v2 0/3] aic79xx: Add some non-NULL checks

[PATCH 6.1 v2 0/3] aic79xx: Add some non-NULL checks

[Boris Belyavtsev]

Add non-NULL checks for ahd_lookup_scb return value.

scb could be NULL if faulty hardware return certain incorrect values to the
driver.

Changes in v2:
1. Reform cover letter text.
2. Fix the mess with style in v1.

Boris Belyavtsev (1):
  scsi: aic79xx: check for non-NULL scb in ahd_handle_seqint

Boris Belyavtsev (2):
  scsi: aic79xx: check for non-NULL scb in ahd_handle_pkt_busfree
  scsi: aic79xx: check for non-NULL scb in ahd_linux_queue_abort_cmd

 drivers/scsi/aic7xxx/aic79xx_core.c | 11 +++++++++--
 drivers/scsi/aic7xxx/aic79xx_osm.c  |  3 ++-
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 6.1 v2 1/3] scsi: aic79xx: check for non-NULL scb in ahd_handle_seqint

[Boris Belyavtsev]

NULL pointer dereference is possible when compiled with AHD_DEBUG and
AHD_SHOW_RECOVERY is set if data in SCBPTR и SCBPTR+1 ports is
incorrect.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Boris Belyavtsev <bbelyavtsev@usergate.com>
---
 drivers/scsi/aic7xxx/aic79xx_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index f9372a81cd4e..a4d5376123d3 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -2205,13 +2205,16 @@ ahd_handle_seqint(struct ahd_softc *ahd, u_int intstat)
 			ahd_print_path(ahd, scb);
 			printk("data overrun detected %s.  Tag == 0x%x.\n",
 			       ahd_lookup_phase_entry(lastphase)->phasemsg,
-			       SCB_GET_TAG(scb));
+			       scb != NULL ? SCB_GET_TAG(scb) : 0);
 			ahd_print_path(ahd, scb);
 			printk("%s seen Data Phase.  Length = %ld.  "
 			       "NumSGs = %d.\n",
 			       ahd_inb(ahd, SEQ_FLAGS) & DPHASE
 			       ? "Have" : "Haven't",
-			       ahd_get_transfer_length(scb), scb->sg_count);
+			       scb != NULL ? ahd_get_transfer_length(scb) : -1,
+			       scb != NULL ? scb->sg_count : -1);
+			if (scb == NULL)
+				break;
 			ahd_dump_sglist(scb);
 		}
 #endif
-- 
2.43.0

[PATCH 6.1 v2 2/3] scsi: aic79xx: check for non-NULL scb in ahd_handle_pkt_busfree

[Boris Belyavtsev]

If hardware returns invalid scbid scb could be NULL.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Boris Belyavtsev <bbelyavtsev@usergate.com>
---
 drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index a4d5376123d3..3595a1e35a69 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -2998,6 +2998,10 @@ ahd_handle_pkt_busfree(struct ahd_softc *ahd, u_int busfreetime)
 		ahd_print_path(ahd, scb);
 		printk("Unexpected PKT busfree condition\n");
 		ahd_dump_card_state(ahd);
+		if (scb == NULL) {
+			printk("scb pointer is NULL\n");
+			return (1);
+		}
 		ahd_abort_scbs(ahd, SCB_GET_TARGET(ahd, scb), 'A',
 			       SCB_GET_LUN(scb), SCB_GET_TAG(scb),
 			       ROLE_INITIATOR, CAM_UNEXP_BUSFREE);
-- 
2.43.0

[PATCH 6.1 v2 3/3] scsi: aic79xx: check for non-NULL scb in ahd_linux_queue_abort_cmd

[Boris Belyavtsev]

possible NULL pointer dereference in case hardware returns invalid scb
index.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 73a254621007 ("[SCSI] aic79xx: update to use scsi_transport_spi")
Signed-off-by: Boris Belyavtsev <bbelyavtsev@usergate.com>
---
 drivers/scsi/aic7xxx/aic79xx_osm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c
index 17dfc3c72110..f2ae202d2641 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -2265,7 +2265,8 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)
 		 * and hope that the target responds.
 		 */
 		pending_scb = ahd_lookup_scb(ahd, active_scbptr);
-		pending_scb->flags |= SCB_RECOVERY_SCB|SCB_ABORT;
+		if (pending_scb != NULL)
+			pending_scb->flags |= SCB_RECOVERY_SCB|SCB_ABORT;
 		ahd_outb(ahd, MSG_OUT, HOST_MSG);
 		ahd_outb(ahd, SCSISIGO, last_phase|ATNO);
 		scmd_printk(KERN_INFO, cmd, "Device is active, asserting ATN\n");
-- 
2.43.0

Re: [PATCH 6.1 v2 0/3] aic79xx: Add some non-NULL checks

[James Bottomley]

On Mon, 2025-04-21 at 15:16 +0700, Boris Belyavtsev wrote:
> Add non-NULL checks for ahd_lookup_scb return value.
> 
> scb could be NULL if faulty hardware return certain incorrect values
> to the driver.

It's a general principle that we trust values coming from the card ...
you are, after all, trusting it with your data.  If there's a fault in
the way the card is operating, we can work around that, so if you have
a card which is producing these NULLs, can you provide details so we
can investigate?

Regards,

James

Re: [PATCH 6.1 v2 0/3] aic79xx: Add some non-NULL checks

[Boris Belyavtsev]

On Mon Apr 21, 2025 at 7:12 PM +07, James Bottomley wrote:
> On Mon, 2025-04-21 at 15:16 +0700, Boris Belyavtsev wrote:
> > Add non-NULL checks for ahd_lookup_scb return value.
> >
> > scb could be NULL if faulty hardware return certain incorrect values
> > to the driver.
>
> It's a general principle that we trust values coming from the card ...
> you are, after all, trusting it with your data.  If there's a fault in
> the way the card is operating, we can work around that, so if you have
> a card which is producing these NULLs, can you provide details so we
> can investigate?
>
> Regards,
>
> James

Well, to be honest, I do not have such a device/card which would
represent the problem. These checks are more about defensive programming
(in case of an accident fault in a card for example).

I agree this checks could be excessive, especially in ahd_linux_queue_abort_cmd()
at aic_79xx_osm.c NULL value is unexpected.

What do you think about that?

Anyways it is up to maintainer if this checks could be valuable here or
not.

Re: [PATCH 6.1 v2 0/3] aic79xx: Add some non-NULL checks

[James Bottomley]

On Mon, 2025-04-28 at 11:32 +0700, Boris Belyavtsev wrote:
> On Mon Apr 21, 2025 at 7:12 PM +07, James Bottomley wrote:
> > On Mon, 2025-04-21 at 15:16 +0700, Boris Belyavtsev wrote:
> > > Add non-NULL checks for ahd_lookup_scb return value.
> > > 
> > > scb could be NULL if faulty hardware return certain incorrect
> > > values to the driver.
> > 
> > It's a general principle that we trust values coming from the card
> > ... you are, after all, trusting it with your data.  If there's a
> > fault in the way the card is operating, we can work around that, so
> > if you have a card which is producing these NULLs, can you provide
> > details so we can investigate?
> > 
> > Regards,
> > 
> > James
> 
> Well, to be honest, I do not have such a device/card which would
> represent the problem. These checks are more about defensive
> programming (in case of an accident fault in a card for example).

We don't program defensively against adapters: they're part of our
trust domain.  We only program defensively against input from untrusted
domains (like user space).  The problem with defensively programming
drivers was nicely demonstrated by the driver hardening project:

https://lore.kernel.org/all/20230119170633.40944-1-alexander.shishkin@linux.intel.com/

In that there are so many ways that drivers could be used to attack the
OS, defending against all of them would dramatically impact the fast
path.  Which is also the reason we don't imagine card faults and then
program for them without first finding the problem in the field.

Regards,

James