[PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[Dinghao Liu]

ata_qc_from_tag() may return a null pointer and further lead to
null-pointer-dereference. Add a return value check to avoid such case.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 drivers/ata/sata_dwc_460ex.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 9dcef6ac643b..0068247ffc06 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -548,8 +548,10 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
 		 * active tag.  It is the tag that matches the command about to
 		 * be completed.
 		 */
-		qc->ap->link.active_tag = tag;
-		sata_dwc_bmdma_start_by_tag(qc, tag);
+		if (qc) {
+			qc->ap->link.active_tag = tag;
+			sata_dwc_bmdma_start_by_tag(qc, tag);
+		}
 
 		handled = 1;
 		goto DONE;
-- 
2.17.1

Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[Andy Shevchenko]

On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
>
> ata_qc_from_tag() may return a null pointer and further lead to
> null-pointer-dereference. Add a return value check to avoid such case.

Can you elaborate more on this? Is it a real case?
I have a hardware, how can I reproduce this?

-- 
With Best Regards,
Andy Shevchenko

Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[dinghao.liu]

> On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> >
> > ata_qc_from_tag() may return a null pointer and further lead to
> > null-pointer-dereference. Add a return value check to avoid such case.
> 
> Can you elaborate more on this? Is it a real case?
> I have a hardware, how can I reproduce this?
> 

In the branch 'if (intpr & SATA_DWC_INTPR_NEWFP)', we call ata_qc_from_tag()
and access qc->ap->link.active_tag immediately. If ata_qc_from_tag() returns
a null pointer, accessing qc->ap->link.active_tag may crash the system.

This issue is reported by my static analysis tool, so I don't have the
vulnerable input currently.

Regards,
Dinghao

Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[Andy Shevchenko]

On Mon, Mar 1, 2021 at 1:20 PM <dinghao.liu@zju.edu.cn> wrote:
>
> > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > >
> > > ata_qc_from_tag() may return a null pointer and further lead to
> > > null-pointer-dereference. Add a return value check to avoid such case.
> >
> > Can you elaborate more on this? Is it a real case?
> > I have a hardware, how can I reproduce this?
> >
>
> In the branch 'if (intpr & SATA_DWC_INTPR_NEWFP)', we call ata_qc_from_tag()
> and access qc->ap->link.active_tag immediately. If ata_qc_from_tag() returns
> a null pointer, accessing qc->ap->link.active_tag may crash the system.

Yes, I can see that. My question is how to get into the case when this
will be true.

> This issue is reported by my static analysis tool, so I don't have the
> vulnerable input currently.

Should we blindly follow everything that some (non-ideal) tool
reports? I don't think so.
For all my experiments with that hardware, I haven't heard about the
issue with NULL pointers. Useless checks make code harder to read and
CPU to waste cycles. It might be maintainers of this driver consider
otherwise, so not my call.

-- 
With Best Regards,
Andy Shevchenko

Re: Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[dinghao.liu]

> On Mon, Mar 1, 2021 at 1:20 PM <dinghao.liu@zju.edu.cn> wrote:
> >
> > > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > > >
> > > > ata_qc_from_tag() may return a null pointer and further lead to
> > > > null-pointer-dereference. Add a return value check to avoid such case.
> > >
> > > Can you elaborate more on this? Is it a real case?
> > > I have a hardware, how can I reproduce this?
> > >
> >
> > In the branch 'if (intpr & SATA_DWC_INTPR_NEWFP)', we call ata_qc_from_tag()
> > and access qc->ap->link.active_tag immediately. If ata_qc_from_tag() returns
> > a null pointer, accessing qc->ap->link.active_tag may crash the system.
> 
> Yes, I can see that. My question is how to get into the case when this
> will be true.
> 

I cannot answer this question immediately. I think it's possible to build 
a designed input to trigger this case for some professional attackers. 

> > This issue is reported by my static analysis tool, so I don't have the
> > vulnerable input currently.
> 
> Should we blindly follow everything that some (non-ideal) tool
> reports? I don't think so.
> For all my experiments with that hardware, I haven't heard about the
> issue with NULL pointers. Useless checks make code harder to read and
> CPU to waste cycles. It might be maintainers of this driver consider
> otherwise, so not my call.
> 

Thanks for your advice. I also checked all use of ata_qc_from_tag() in the 
whole kernel and found all of them had return value checks except for the 
calls in sata_dwc_isr(), which is odd. There is no issue currently does not
mean it will never happen in the future. So I suggest the maintainer of function
sata_dwc_isr() to fix this issue.

Regards,
Dinghao

Re: Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[Andy Shevchenko]

On Tue, Mar 2, 2021 at 9:34 AM <dinghao.liu@zju.edu.cn> wrote:
> > On Mon, Mar 1, 2021 at 1:20 PM <dinghao.liu@zju.edu.cn> wrote:
> > > > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:

...

> > > This issue is reported by my static analysis tool, so I don't have the
> > > vulnerable input currently.
> >
> > Should we blindly follow everything that some (non-ideal) tool
> > reports? I don't think so.
> > For all my experiments with that hardware, I haven't heard about the
> > issue with NULL pointers. Useless checks make code harder to read and
> > CPU to waste cycles. It might be maintainers of this driver consider
> > otherwise, so not my call.
> >
>
> Thanks for your advice. I also checked all use of ata_qc_from_tag() in the
> whole kernel and found all of them had return value checks except for the
> calls in sata_dwc_isr(), which is odd.

Thanks for this information, it makes sense to me. Perhaps you need to
put this into the commit message to justify the need of the change.

> There is no issue currently does not
> mean it will never happen in the future. So I suggest the maintainer of function
> sata_dwc_isr() to fix this issue.



-- 
With Best Regards,
Andy Shevchenko

Re: Re: Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr

[dinghao.liu]

> On Tue, Mar 2, 2021 at 9:34 AM <dinghao.liu@zju.edu.cn> wrote:
> > > On Mon, Mar 1, 2021 at 1:20 PM <dinghao.liu@zju.edu.cn> wrote:
> > > > > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> 
> ...
> 
> > > > This issue is reported by my static analysis tool, so I don't have the
> > > > vulnerable input currently.
> > >
> > > Should we blindly follow everything that some (non-ideal) tool
> > > reports? I don't think so.
> > > For all my experiments with that hardware, I haven't heard about the
> > > issue with NULL pointers. Useless checks make code harder to read and
> > > CPU to waste cycles. It might be maintainers of this driver consider
> > > otherwise, so not my call.
> > >
> >
> > Thanks for your advice. I also checked all use of ata_qc_from_tag() in the
> > whole kernel and found all of them had return value checks except for the
> > calls in sata_dwc_isr(), which is odd.
> 
> Thanks for this information, it makes sense to me. Perhaps you need to
> put this into the commit message to justify the need of the change.
> 

OK. I will fix this and send a new patch soon.

Regards,
Dinghao