| Message ID | 20201005213149.12332-1-sudipm.mukherjee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: host: ehci-sched: avoid possible NULL dereference | expand |
On Monday, October 5, 2020 5:31 PM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote: > find_tt() can return NULL or the error value in ERR_PTR() and > dereferencing the return value without checking for the error can > lead to a possible dereference of NULL pointer or ERR_PTR(). Looks fine to me. There is in fact no checks of the return value before a dereference here, and this solves that. Reviewed-by: Harley A.W. Lorenzo <hl1998@protonmail.com -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#66): https://lists.elisa.tech/g/linux-safety/message/66 Mute This Topic: https://lists.elisa.tech/mt/77331459/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, Oct 05, 2020 at 11:19:02PM +0000, Harley A.W. Lorenzo wrote: > On Monday, October 5, 2020 5:31 PM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote: > > > find_tt() can return NULL or the error value in ERR_PTR() and > > dereferencing the return value without checking for the error can > > lead to a possible dereference of NULL pointer or ERR_PTR(). > > Looks fine to me. There is in fact no checks of the return value > before a dereference here, and this solves that. > > Reviewed-by: Harley A.W. Lorenzo <hl1998@protonmail.com No, this patch is wrong. In fact, these calls to find_tt() cannot return NULL or an ERR_PTR value. Alan Stern -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#68): https://lists.elisa.tech/g/linux-safety/message/68 Mute This Topic: https://lists.elisa.tech/mt/77331459/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, Oct 05, 2020 at 11:19:02PM +0000, Harley A.W. Lorenzo wrote: > On Monday, October 5, 2020 5:31 PM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote: > > > find_tt() can return NULL or the error value in ERR_PTR() and > > dereferencing the return value without checking for the error can > > lead to a possible dereference of NULL pointer or ERR_PTR(). > > Looks fine to me. There is in fact no checks of the return value > before a dereference here, and this solves that. > > Reviewed-by: Harley A.W. Lorenzo <hl1998@protonmail.com Nit, in the future, you need the trailing '>' there. thanks, greg k-h -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#69): https://lists.elisa.tech/g/linux-safety/message/69 Mute This Topic: https://lists.elisa.tech/mt/77331459/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi All, I sent out this patch yesterday which I think is an obvious safety issue as the error was not handled. This is a change in code and the resultant binary will not be same. I know we are moving to pcie now, but have we decided yet how to validate these kinds of changes?
On Mon, Oct 05, 2020 at 09:25:44PM -0400, stern@rowland.harvard.edu wrote: > On Mon, Oct 05, 2020 at 11:19:02PM +0000, Harley A.W. Lorenzo wrote: > > On Monday, October 5, 2020 5:31 PM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote: > > > > > find_tt() can return NULL or the error value in ERR_PTR() and > > > dereferencing the return value without checking for the error can > > > lead to a possible dereference of NULL pointer or ERR_PTR(). > > > > Looks fine to me. There is in fact no checks of the return value > > before a dereference here, and this solves that. > > > > Reviewed-by: Harley A.W. Lorenzo <hl1998@protonmail.com > > No, this patch is wrong. In fact, these calls to find_tt() cannot > return NULL or an ERR_PTR value. Sudip, if you would prefer to submit a patch that adds comments to those call sites explaining that find_tt() will not return NULL or an error, that would be okay. Alan Stern -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#70): https://lists.elisa.tech/g/linux-safety/message/70 Mute This Topic: https://lists.elisa.tech/mt/77331459/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c index 6dfb242f9a4b..f3fd7e9fe6b2 100644 --- a/drivers/usb/host/ehci-sched.c +++ b/drivers/usb/host/ehci-sched.c @@ -245,6 +245,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci, /* FS/LS bus bandwidth */ if (tt_usecs) { tt = find_tt(qh->ps.udev); + if (IS_ERR_OR_NULL(tt)) + return; if (sign > 0) list_add_tail(&qh->ps.ps_list, &tt->ps_list); else @@ -1338,6 +1340,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci, } tt = find_tt(stream->ps.udev); + if (IS_ERR_OR_NULL(tt)) + return; if (sign > 0) list_add_tail(&stream->ps.ps_list, &tt->ps_list); else
find_tt() can return NULL or the error value in ERR_PTR() and dereferencing the return value without checking for the error can lead to a possible dereference of NULL pointer or ERR_PTR(). Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> --- drivers/usb/host/ehci-sched.c | 4 ++++ 1 file changed, 4 insertions(+)