Linux-Tegra Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] usb: tegra: Fix allocation for the FPCI context
@ 2020-07-12 10:28 Jon Hunter
       [not found] ` <20200712102837.24340-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2020-07-14  9:19 ` [PATCH 1/2] " Thierry Reding
  0 siblings, 2 replies; 8+ messages in thread
From: Jon Hunter @ 2020-07-12 10:28 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Mathias Nyman, Greg Kroah-Hartman, linux-tegra, linux-kernel,
	Jon Hunter, stable

Commit 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB
context save/restore") is using the IPFS 'num_offsets' value when
allocating memory for FPCI context instead of the FPCI 'num_offsets'.
We have not observed any specific issues because of this, but could
cause too much memory or too little memory to be allocated. Fix this
by using the FPCI 'num_offsets' for allocating the FPCI memory for
storing the FPCI state.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 9ce28ab47f4b..014d79334f50 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -856,7 +856,7 @@ static int tegra_xusb_init_context(struct tegra_xusb *tegra)
 	if (!tegra->context.ipfs)
 		return -ENOMEM;
 
-	tegra->context.fpci = devm_kcalloc(tegra->dev, soc->ipfs.num_offsets,
+	tegra->context.fpci = devm_kcalloc(tegra->dev, soc->fpci.num_offsets,
 					   sizeof(u32), GFP_KERNEL);
 	if (!tegra->context.fpci)
 		return -ENOMEM;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] usb: tegra: Fix zero length memory allocation
       [not found] ` <20200712102837.24340-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2020-07-12 10:28   ` Jon Hunter
  2020-07-14  9:32     ` Thierry Reding
  2020-07-15 11:38   ` [PATCH V2] usb: tegra: Fix allocation for the FPCI context Jon Hunter
  1 sibling, 1 reply; 8+ messages in thread
From: Jon Hunter @ 2020-07-12 10:28 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Mathias Nyman, Greg Kroah-Hartman,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jon Hunter,
	stable-u79uwXL29TY76Z2rM5mHXA

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore")

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/host/xhci-tegra.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

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;
 }
-- 
2.17.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] usb: tegra: Fix allocation for the FPCI context
  2020-07-12 10:28 [PATCH 1/2] usb: tegra: Fix allocation for the FPCI context Jon Hunter
       [not found] ` <20200712102837.24340-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2020-07-14  9:19 ` Thierry Reding
  1 sibling, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2020-07-14  9:19 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Mathias Nyman, Greg Kroah-Hartman, linux-tegra, linux-kernel, stable


[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Sun, Jul 12, 2020 at 11:28:36AM +0100, Jon Hunter wrote:
> Commit 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB
> context save/restore") is using the IPFS 'num_offsets' value when
> allocating memory for FPCI context instead of the FPCI 'num_offsets'.
> We have not observed any specific issues because of this, but could
> cause too much memory or too little memory to be allocated. Fix this
> by using the FPCI 'num_offsets' for allocating the FPCI memory for
> storing the FPCI state.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Good catch!

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] usb: tegra: Fix zero length memory allocation
  2020-07-12 10:28   ` [PATCH 2/2] usb: tegra: Fix zero length memory allocation Jon Hunter
@ 2020-07-14  9:32     ` Thierry Reding
  2020-07-15  8:43       ` Jon Hunter
  0 siblings, 1 reply; 8+ messages in thread
From: Thierry Reding @ 2020-07-14  9:32 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Mathias Nyman, Greg Kroah-Hartman, linux-tegra, linux-kernel, stable


[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] usb: tegra: Fix zero length memory allocation
  2020-07-14  9:32     ` Thierry Reding
@ 2020-07-15  8:43       ` Jon Hunter
  0 siblings, 0 replies; 8+ messages in thread
From: Jon Hunter @ 2020-07-15  8:43 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Mathias Nyman, Greg Kroah-Hartman,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA


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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>
>> Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore")
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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

-- 
nvpublic

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH V2] usb: tegra: Fix allocation for the FPCI context
       [not found] ` <20200712102837.24340-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2020-07-12 10:28   ` [PATCH 2/2] usb: tegra: Fix zero length memory allocation Jon Hunter
@ 2020-07-15 11:38   ` Jon Hunter
       [not found]     ` <20200715113842.30680-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  1 sibling, 1 reply; 8+ messages in thread
From: Jon Hunter @ 2020-07-15 11:38 UTC (permalink / raw)
  To: Mathias Nyman, Greg Kroah-Hartman, Thierry Reding
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jon Hunter,
	stable-u79uwXL29TY76Z2rM5mHXA

Commit 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB
context save/restore") is using the IPFS 'num_offsets' value when
allocating memory for FPCI context instead of the FPCI 'num_offsets'.

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
attempting the save the USB context. On Tegra186, the IPFS context has a
zero length but the FPCI content has a non-zero length, and because of
the bug in the Tegra XHCI driver we are incorrectly allocating a zero
length array for the FPCI context. The crash seen on entering suspend
when we attempt to save the FPCI context and following commit
cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") this now
causes a NULL pointer deference when we access the memory. Fix this by
correcting the amount of memory we are allocating for FPCI contexts.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore")

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

Changes since V1:
- Corrected commit message
- Added Thierry's ACK

 drivers/usb/host/xhci-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 9ce28ab47f4b..014d79334f50 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -856,7 +856,7 @@ static int tegra_xusb_init_context(struct tegra_xusb *tegra)
 	if (!tegra->context.ipfs)
 		return -ENOMEM;
 
-	tegra->context.fpci = devm_kcalloc(tegra->dev, soc->ipfs.num_offsets,
+	tegra->context.fpci = devm_kcalloc(tegra->dev, soc->fpci.num_offsets,
 					   sizeof(u32), GFP_KERNEL);
 	if (!tegra->context.fpci)
 		return -ENOMEM;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] usb: tegra: Fix allocation for the FPCI context
       [not found]     ` <20200715113842.30680-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2020-07-23 11:19       ` Greg Kroah-Hartman
  2020-07-29 10:08         ` Jon Hunter
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-23 11:19 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Mathias Nyman, Thierry Reding,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	stable-u79uwXL29TY76Z2rM5mHXA

On Wed, Jul 15, 2020 at 12:38:42PM +0100, Jon Hunter wrote:
> Commit 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB
> context save/restore") is using the IPFS 'num_offsets' value when
> allocating memory for FPCI context instead of the FPCI 'num_offsets'.
> 
> 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
> attempting the save the USB context. On Tegra186, the IPFS context has a
> zero length but the FPCI content has a non-zero length, and because of
> the bug in the Tegra XHCI driver we are incorrectly allocating a zero
> length array for the FPCI context. The crash seen on entering suspend
> when we attempt to save the FPCI context and following commit
> cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") this now
> causes a NULL pointer deference when we access the memory. Fix this by
> correcting the amount of memory we are allocating for FPCI contexts.
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> Fixes: 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB context save/restore")
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> Changes since V1:
> - Corrected commit message
> - Added Thierry's ACK
> 
>  drivers/usb/host/xhci-tegra.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

No cc: to linux-usb@vger?  :(

I'll go queue this up, but I would have caught it sooner if you had done
so...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V2] usb: tegra: Fix allocation for the FPCI context
  2020-07-23 11:19       ` Greg Kroah-Hartman
@ 2020-07-29 10:08         ` Jon Hunter
  0 siblings, 0 replies; 8+ messages in thread
From: Jon Hunter @ 2020-07-29 10:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Mathias Nyman, Thierry Reding, linux-tegra, linux-kernel, stable


On 23/07/2020 12:19, Greg Kroah-Hartman wrote:
> On Wed, Jul 15, 2020 at 12:38:42PM +0100, Jon Hunter wrote:
>> Commit 5c4e8d3781bc ("usb: host: xhci-tegra: Add support for XUSB
>> context save/restore") is using the IPFS 'num_offsets' value when
>> allocating memory for FPCI context instead of the FPCI 'num_offsets'.
>>
>> 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
>> attempting the save the USB context. On Tegra186, the IPFS context has a
>> zero length but the FPCI content has a non-zero length, and because of
>> the bug in the Tegra XHCI driver we are incorrectly allocating a zero
>> length array for the FPCI context. The crash seen on entering suspend
>> when we attempt to save the FPCI context and following commit
>> cad064f1bd52 ("devres: handle zero size in devm_kmalloc()") this now
>> causes a NULL pointer deference when we access the memory. Fix this by
>> correcting the amount of memory we are allocating for FPCI contexts.
>>
>> 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>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> ---
>>
>> Changes since V1:
>> - Corrected commit message
>> - Added Thierry's ACK
>>
>>  drivers/usb/host/xhci-tegra.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> No cc: to linux-usb@vger?  :(
> 
> I'll go queue this up, but I would have caught it sooner if you had done
> so...

Sorry about that. Thanks for queuing up!
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-12 10:28 [PATCH 1/2] usb: tegra: Fix allocation for the FPCI context Jon Hunter
     [not found] ` <20200712102837.24340-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2020-07-12 10:28   ` [PATCH 2/2] usb: tegra: Fix zero length memory allocation Jon Hunter
2020-07-14  9:32     ` Thierry Reding
2020-07-15  8:43       ` Jon Hunter
2020-07-15 11:38   ` [PATCH V2] usb: tegra: Fix allocation for the FPCI context Jon Hunter
     [not found]     ` <20200715113842.30680-1-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2020-07-23 11:19       ` Greg Kroah-Hartman
2020-07-29 10:08         ` Jon Hunter
2020-07-14  9:19 ` [PATCH 1/2] " Thierry Reding

Linux-Tegra Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-tegra/0 linux-tegra/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-tegra linux-tegra/ https://lore.kernel.org/linux-tegra \
		linux-tegra@vger.kernel.org
	public-inbox-index linux-tegra

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-tegra


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git