From: GuoHan Zhao <zhaoguohan@kylinos.cn>

Fix a null pointer dereference issue.

The code dereferences s->current before checking if it is NULL. Move the
null check before the dereference to prevent potential crashes.

This issue could occur if s->current is NULL when the function reaches
the "Host adapter (re)connected" path, though this should not normally
happen during correct operation.

Fixes: 9ce93b74cdc0 ("ncr710: Add driver for the NCR 53c710 SCSI chip")
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
 hw/scsi/ncr53c710.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c
index ade951b1d107..e479a212bc54 100644
--- a/hw/scsi/ncr53c710.c
+++ b/hw/scsi/ncr53c710.c
@@ -831,14 +831,14 @@ void ncr710_transfer_data(SCSIRequest *req, uint32_t len)
         }
     }
 
-    /* Host adapter (re)connected */
-    s->current->dma_len = len;
-    s->command_complete = NCR710_CMD_DATA_READY;
-
     if (!s->current) {
         return;
     }
 
+    /* Host adapter (re)connected */
+    s->current->dma_len = len;
+    s->command_complete = NCR710_CMD_DATA_READY;
+
     if (s->waiting) {
         s->scntl1 |= NCR710_SCNTL1_CON;
         s->istat |= NCR710_ISTAT_CON;
-- 
2.43.0

On 10/31/25 04:24, zhaoguohan_salmon@163.com wrote:
> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> 
> Fix a null pointer dereference issue.
> 
> The code dereferences s->current before checking if it is NULL. Move the
> null check before the dereference to prevent potential crashes.
> 
> This issue could occur if s->current is NULL when the function reaches
> the "Host adapter (re)connected" path, though this should not normally
> happen during correct operation.
> 
> Fixes: 9ce93b74cdc0 ("ncr710: Add driver for the NCR 53c710 SCSI chip")
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> ---
>   hw/scsi/ncr53c710.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c
> index ade951b1d107..e479a212bc54 100644
> --- a/hw/scsi/ncr53c710.c
> +++ b/hw/scsi/ncr53c710.c
> @@ -831,14 +831,14 @@ void ncr710_transfer_data(SCSIRequest *req, uint32_t len)
>           }
>       }
>   
> -    /* Host adapter (re)connected */
> -    s->current->dma_len = len;
> -    s->command_complete = NCR710_CMD_DATA_READY;

^^
I wonder if you need to keep s->command_complete, and not move it...
Maybe only dma_len needs to be protected?
I added Soumyajyotii to Cc here...


>       if (!s->current) {
>           return;
>       }
>   
> +    /* Host adapter (re)connected */
> +    s->current->dma_len = len;
> +    s->command_complete = NCR710_CMD_DATA_READY;

On Fri, Oct 31, 2025 at 9:39 PM Helge Deller <deller@gmx.de> wrote:

> On 10/31/25 04:24, zhaoguohan_salmon@163.com wrote:
> > From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> >
> > Fix a null pointer dereference issue.
> >
> > The code dereferences s->current before checking if it is NULL. Move the
> > null check before the dereference to prevent potential crashes.
> >
> > This issue could occur if s->current is NULL when the function reaches
> > the "Host adapter (re)connected" path, though this should not normally
> > happen during correct operation.
> >
> > Fixes: 9ce93b74cdc0 ("ncr710: Add driver for the NCR 53c710 SCSI chip")
> > Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> > ---
> >   hw/scsi/ncr53c710.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c
> > index ade951b1d107..e479a212bc54 100644
> > --- a/hw/scsi/ncr53c710.c
> > +++ b/hw/scsi/ncr53c710.c
> > @@ -831,14 +831,14 @@ void ncr710_transfer_data(SCSIRequest *req,
> uint32_t len)
> >           }
> >       }
> >
> > -    /* Host adapter (re)connected */
> > -    s->current->dma_len = len;
> > -    s->command_complete = NCR710_CMD_DATA_READY;
>
> ^^
> I wonder if you need to keep s->command_complete, and not move it...
> Maybe only dma_len needs to be protected?
> I added Soumyajyotii to Cc here...
>

Hello GuoHan,
Thank you for pointing out the issue and suggesting the fix.
Unfortunately, with the changes you have suggested, it seems to fail to
boot on my setup while
I tried to reproduce it.
I think moving it below might break the boot process, as we never set the
s->command_complete properly.
I believe as Helge suggested only s->current->dma_len needs to be null
protected.

So i did something along the lines of::
-    /* Host adapter (re)connected */
-    s->current->dma_len = len;
     s->command_complete = NCR710_CMD_DATA_READY;
-
     if (!s->current) {
-        return;
+        s->current = (NCR710Request *)req->hba_private;
     }
+    s->current->dma_len = len;
 and it seems to work.

Thank you,
Soumyajyotii


> >       if (!s->current) {
> >           return;
> >       }
> >
> > +    /* Host adapter (re)connected */
> > +    s->current->dma_len = len;
> > +    s->command_complete = NCR710_CMD_DATA_READY;
>
>