| Message ID | 20210301072842.7410-1-dinghao.liu@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | sata_dwc_460ex: Fix missing check in sata_dwc_isr | expand |
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?
> 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
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.
> 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
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.
> 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
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;
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(-)