| Message ID | 20200712102837.24340-2-jonathanh@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] usb: tegra: Fix allocation for the FPCI context | expand |
On Sun, Jul 12, 2020 at 11:28:37AM +0100, Jon Hunter wrote: > After commit cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") > was added system suspend started failing on Tegra186. The kernel log > showed that the Tegra XHCI driver was crashing on entry to suspend when > attemptin the save the USB context. The problem is caused because we > are trying to allocate a zero length array for the IPFS context on > Tegra186 and following commit cad064f1bd52 ("devres: handle zero size > in devm_kmalloc()") this now causes a NULL pointer deference crash > when we try to access the memory. Fix this by only allocating memory > for both the IPFS and FPCI contexts when required. > > Cc: stable@vger.kernel.org > > Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore") > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/usb/host/xhci-tegra.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) Actually it would seem to me that this is no longer a bug after your fix in patch 1. We only ever access tegra->context.ipfs if tegra->soc->ipfs.num_offsets > 0, so the special ZERO_SIZE_PTR case will not actually cause an issue anymore. The reason why this was crashing was because tegra->context.fpci was allocated with a zero size (because of the bug that you fixed in patch 1) and then that zero-size pointer was dereferenced because the code was correctly checking for tegra->soc->fpci.num_offsets > 0 in the context save and restore. So I don't think there's a bug here. It's not wrong to allocate a zero- size buffer. It's only a bug to then go and dereference it. Are you still seeing the issue if you leave out this patch and only apply patch 1? Thierry
On 14/07/2020 10:32, Thierry Reding wrote: > On Sun, Jul 12, 2020 at 11:28:37AM +0100, Jon Hunter wrote: >> After commit cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") >> was added system suspend started failing on Tegra186. The kernel log >> showed that the Tegra XHCI driver was crashing on entry to suspend when >> attemptin the save the USB context. The problem is caused because we >> are trying to allocate a zero length array for the IPFS context on >> Tegra186 and following commit cad064f1bd52 ("devres: handle zero size >> in devm_kmalloc()") this now causes a NULL pointer deference crash >> when we try to access the memory. Fix this by only allocating memory >> for both the IPFS and FPCI contexts when required. >> >> Cc: stable@vger.kernel.org >> >> Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore") >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/usb/host/xhci-tegra.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) > > Actually it would seem to me that this is no longer a bug after your fix > in patch 1. We only ever access tegra->context.ipfs if > tegra->soc->ipfs.num_offsets > 0, so the special ZERO_SIZE_PTR case will > not actually cause an issue anymore. > > The reason why this was crashing was because tegra->context.fpci was > allocated with a zero size (because of the bug that you fixed in patch > 1) and then that zero-size pointer was dereferenced because the code was > correctly checking for tegra->soc->fpci.num_offsets > 0 in the context > save and restore. > > So I don't think there's a bug here. It's not wrong to allocate a zero- > size buffer. It's only a bug to then go and dereference it. Are you > still seeing the issue if you leave out this patch and only apply patch > 1? Ah yes you are right. OK, we can drop this. I will update the commit message to patch 1/1. Jon
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index 014d79334f50..b2e4e1c128b0 100644 --- a/drivers/usb/host/xhci-tegra.c +++ b/drivers/usb/host/xhci-tegra.c @@ -851,15 +851,21 @@ static int tegra_xusb_init_context(struct tegra_xusb *tegra) { const struct tegra_xusb_context_soc *soc = tegra->soc->context; - tegra->context.ipfs = devm_kcalloc(tegra->dev, soc->ipfs.num_offsets, - sizeof(u32), GFP_KERNEL); - if (!tegra->context.ipfs) - return -ENOMEM; + if (soc->ipfs.num_offsets > 0) { + tegra->context.ipfs = devm_kcalloc(tegra->dev, + soc->ipfs.num_offsets, + sizeof(u32), GFP_KERNEL); + if (!tegra->context.ipfs) + return -ENOMEM; + } - tegra->context.fpci = devm_kcalloc(tegra->dev, soc->fpci.num_offsets, - sizeof(u32), GFP_KERNEL); - if (!tegra->context.fpci) - return -ENOMEM; + if (soc->fpci.num_offsets > 0) { + tegra->context.fpci = devm_kcalloc(tegra->dev, + soc->fpci.num_offsets, + sizeof(u32), GFP_KERNEL); + if (!tegra->context.fpci) + return -ENOMEM; + } return 0; }
After commit cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") was added system suspend started failing on Tegra186. The kernel log showed that the Tegra XHCI driver was crashing on entry to suspend when attemptin the save the USB context. The problem is caused because we are trying to allocate a zero length array for the IPFS context on Tegra186 and following commit cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") this now causes a NULL pointer deference crash when we try to access the memory. Fix this by only allocating memory for both the IPFS and FPCI contexts when required. Cc: stable@vger.kernel.org Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/usb/host/xhci-tegra.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)