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

On Mon, Mar 1, 2021 at 1:20 PM [off-list ref] wrote:

quoted

quoted

On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu [off-list ref] wrote:

quoted

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. 

quoted

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