| Message ID | 1652891743-110930-1-git-send-email-lyz_cs@pku.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | usb: cdnsp: Fix potential dereference of NULL pointer | expand |
On Wed, May 18, 2022 at 09:35:43AM -0700, Yongzhi Liu wrote: > The return value of cdnsp_get_transfer_ring() > needs to be checked to avoid use of NULL pointer > in case of an acquisition failure. Please use the full 72 columns > > Fixes: 3d8290455 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> Please do not put a blank line between "Fixes:" and your signed off by line. > --- > drivers/usb/cdns3/cdnsp-ring.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > index 1b14384..9f206b9 100644 > --- a/drivers/usb/cdns3/cdnsp-ring.c > +++ b/drivers/usb/cdns3/cdnsp-ring.c > @@ -655,6 +655,8 @@ static int cdnsp_cmd_set_deq(struct cdnsp_device *pdev, > * to reflect the new position. > */ > ep_ring = cdnsp_get_transfer_ring(pdev, pep, deq_state->stream_id); > + if (!ep_ring) > + return -EINVAL; How did you test this? Don't you need to properly clean up and handle the hardware issues if you exit early here? Without good testing, I am loath to take this. thanks, greg k-h
> -----Original Messages----- > From: "Greg KH" <gregkh@linuxfoundation.org> > Sent Time: 2022-05-19 23:58:35 (Thursday) > To: "Yongzhi Liu" <lyz_cs@pku.edu.cn> > Cc: pawell@cadence.com, peter.chen@nxp.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, fuyq@stu.pku.edu.cn > Subject: Re: [PATCH] usb: cdnsp: Fix potential dereference of NULL pointer > > On Wed, May 18, 2022 at 09:35:43AM -0700, Yongzhi Liu wrote: > > The return value of cdnsp_get_transfer_ring() > > needs to be checked to avoid use of NULL pointer > > in case of an acquisition failure. > > Please use the full 72 columns > > > > > Fixes: 3d8290455 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > > > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > > Please do not put a blank line between "Fixes:" and your signed off by > line. > > > --- > > drivers/usb/cdns3/cdnsp-ring.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > > index 1b14384..9f206b9 100644 > > --- a/drivers/usb/cdns3/cdnsp-ring.c > > +++ b/drivers/usb/cdns3/cdnsp-ring.c > > @@ -655,6 +655,8 @@ static int cdnsp_cmd_set_deq(struct cdnsp_device *pdev, > > * to reflect the new position. > > */ > > ep_ring = cdnsp_get_transfer_ring(pdev, pep, deq_state->stream_id); > > + if (!ep_ring) > > + return -EINVAL; > > How did you test this? > > Don't you need to properly clean up and handle the hardware issues if > you exit early here? I find this by a static analyzer based on frequency and similarity, which report many null ptr deref bugs. In cdns3/cdnsp-ring.c, I find that we usually check the return value when call function 'usb_get_intfdata'. If 'deq_state->stream_id' is special, the 'ep_ring' is also null. Therefore, i think we should add null checks here. I will resubmit a new patch if you think the bug is real. Thanks for your reply and advice. > > Without good testing, I am loath to take this. > > thanks, > > greg k-h
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c index 1b14384..9f206b9 100644 --- a/drivers/usb/cdns3/cdnsp-ring.c +++ b/drivers/usb/cdns3/cdnsp-ring.c @@ -655,6 +655,8 @@ static int cdnsp_cmd_set_deq(struct cdnsp_device *pdev, * to reflect the new position. */ ep_ring = cdnsp_get_transfer_ring(pdev, pep, deq_state->stream_id); + if (!ep_ring) + return -EINVAL; if (cdnsp_trb_is_link(ep_ring->dequeue)) { ep_ring->deq_seg = ep_ring->deq_seg->next;
The return value of cdnsp_get_transfer_ring() needs to be checked to avoid use of NULL pointer in case of an acquisition failure. Fixes: 3d8290455 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/usb/cdns3/cdnsp-ring.c | 2 ++ 1 file changed, 2 insertions(+)