[U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
The crash occurs due to below commit[1], revert of this patch resolves the issue.
[1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b Author: Masahiro Yamada yamada.masahiro@socionext.com Date: Tue Jun 28 10:48:40 2016 +0900
types.h: move and redefine resource_size_t
Currently, this is only defined in arch/arm/include/asm/types.h, so move it to include/linux/types.h to make it available for all architectures.
I defined it with phys_addr_t as Linux does. I needed to surround the define with #ifdef __KERNEL__ ... #endif to avoid build errors in tools building. (Host tools should not include <linux/types.h> in the first place, but this is already messy in U-Boot...)
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Simon Glass sjg@chromium.org
Signed-off-by: Ravi Babu ravibabu@ti.com --- drivers/usb/dwc3/ep0.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 12b133f..f49a06e 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -14,6 +14,7 @@ * SPDX-License-Identifier: GPL-2.0 */
+#include <common.h> #include <linux/kernel.h> #include <linux/list.h>
On 07/21/2016 12:41 PM, Ravi Babu wrote:
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Make the explanation terse, it took me quite a while to extrapolate the message from the text.
The crash occurs due to below commit[1], revert of this patch resolves the issue.
[1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b Author: Masahiro Yamada yamada.masahiro@socionext.com Date: Tue Jun 28 10:48:40 2016 +0900
types.h: move and redefine resource_size_t
No need to include the whole commit message of another commit, just the subject is enough. Also, I dunno why you add two levels of indent to the headers of the commit, but not to the subject, this is real confusing.
Currently, this is only defined in arch/arm/include/asm/types.h, so move it to include/linux/types.h to make it available for all architectures.
I defined it with phys_addr_t as Linux does. I needed to surround the define with #ifdef __KERNEL__ ... #endif to avoid build errors in tools building. (Host tools should not include <linux/types.h> in the first place, but this is already messy in U-Boot...)
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Simon Glass sjg@chromium.org
Signed-off-by: Ravi Babu ravibabu@ti.com
drivers/usb/dwc3/ep0.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 12b133f..f49a06e 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -14,6 +14,7 @@
- SPDX-License-Identifier: GPL-2.0
*/
+#include <common.h> #include <linux/kernel.h> #include <linux/list.h>
Hi Marek
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
Make the explanation terse, it took me quite a while to extrapolate the message from the text.
The crash occurs due to below commit[1], revert of this patch resolves the issue.
[1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b Author: Masahiro Yamada yamada.masahiro@socionext.com Date: Tue Jun 28 10:48:40 2016 +0900
types.h: move and redefine resource_size_t
No need to include the whole commit message of another commit, just the subject is enough. Also, I dunno why you add two levels of indent to the headers of the commit, but not to the subject, this is real confusing.
My bad, sorry for causing confusion.
Regards Ravi
On 07/21/2016 02:29 PM, B, Ravi wrote:
Hi Marek
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
Also, please fix your mailer to break at 80 chars per line.
Make the explanation terse, it took me quite a while to extrapolate the message from the text.
The crash occurs due to below commit[1], revert of this patch resolves the issue.
[1] commit 95ebc253e6d4a3370e3dab14743bfc99fcd9cf1b Author: Masahiro Yamada yamada.masahiro@socionext.com Date: Tue Jun 28 10:48:40 2016 +0900
types.h: move and redefine resource_size_t
No need to include the whole commit message of another commit, just the subject is enough. Also, I dunno why you add two levels of indent to the headers of the commit, but not to the subject, this is real confusing.
My bad, sorry for causing confusion.
Regards Ravi
Hi Marek
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure defined in drivers/usb/dwc3/core.h
struct dwc3 { .... struct resource xhci_resource[DWC3_XHCI_RESOURCES_NUM]; .. }; The resource structure defined in include/linux/ioport.h struct resource { resource_size_t start; resource_size_t end; }
Also, please fix your mailer to break at 80 chars per line.
Sure.
Regards Ravi
On 07/21/2016 02:44 PM, B, Ravi wrote:
Hi Marek
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure defined in drivers/usb/dwc3/core.h
struct dwc3 { .... struct resource xhci_resource[DWC3_XHCI_RESOURCES_NUM]; .. }; The resource structure defined in include/linux/ioport.h struct resource { resource_size_t start; resource_size_t end; }
OK, I am starting to get a better picture of this issue. But somehow I suspect there is a deeper problem with the includes -- how is it possible that the compiler didn't complain about the resource_size_t being undefined, but instead used (incorrectly sized) variant ?
Also, please fix your mailer to break at 80 chars per line.
Sure.
Regards Ravi
Hi Marek
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure defined in drivers/usb/dwc3/core.h
struct dwc3 { .... struct resource xhci_resource[DWC3_XHCI_RESOURCES_NUM]; .. }; The resource structure defined in include/linux/ioport.h struct resource { resource_size_t start; resource_size_t end; }
OK, I am starting to get a better picture of this issue. But somehow I suspect there is a deeper problem with the includes -- how is it possible that the compiler didn't complain about the resource_size_t being undefined, but instead used (incorrectly sized) variant ?
Yeah, I agree. Compiler not complaining, may be due to fallback to old definitions (32 bit) of resource_size_t because of right header file is not included.
Regards Ravi
On 07/21/2016 03:03 PM, B, Ravi wrote:
Hi Marek
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due >to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which >leads to pointing to wrong offset location causing the crash.
This stuff should be in the commit message. Still, git grep resource_size_t does not show that it's used in gadget.c , so I don't understand how this patch can fix things.
Thanks, I will add this text to commit message. The resource_size_t is used in dwc3 structure defined in drivers/usb/dwc3/core.h
struct dwc3 { .... struct resource xhci_resource[DWC3_XHCI_RESOURCES_NUM]; .. }; The resource structure defined in include/linux/ioport.h struct resource { resource_size_t start; resource_size_t end; }
OK, I am starting to get a better picture of this issue. But somehow I suspect there is a deeper problem with the includes -- how is it possible that the compiler didn't complain about the resource_size_t being undefined, but instead used (incorrectly sized) variant ?
Yeah, I agree. Compiler not complaining, may be due to fallback to old definitions (32 bit) of resource_size_t because of right header file is not included.
Well please look into it and properly explain why this happens.
And fix your damned mailer !
Regards Ravi
Hi.
2016-07-21 21:29 GMT+09:00 B, Ravi ravibabu@ti.com:
Hi Marek
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
I will explain it more precisely.
Both gadget.c and ep.c include <linux/types.h>
See, include/linux/types.h is included from include/linux/kernel.h included from drivers/usb/dwc3/ep0.c
So, <linux/types.h> is not a problem.
The root cause of problem is: gadget.c include <config.h>, but ep0.c does not.
If <config.h> is not included, any CONFIGs from the board header are defined.
The size of phys_addr_t depends on CONFIG_PHYS_64BIT as you see in:
#ifdef CONFIG_PHYS_64BIT typedef unsigned long long phys_addr_t; typedef unsigned long long phys_size_t; #else /* DMA addresses are 32-bits wide */ typedef unsigned long phys_addr_t; typedef unsigned long phys_size_t; #endif
So, phys_addr_t is 8 byte in gadget.c and phys_addr_t is 4 byte in ep0.c
My commit changed resource_size_t based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Hi Masahiro-san
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
I will explain it more precisely. Both gadget.c and ep.c include <linux/types.h> See, include/linux/types.h is included from include/linux/kernel.h included from drivers/usb/dwc3/ep0.c
So, <linux/types.h> is not a problem.
The root cause of problem is: gadget.c include <config.h>, but ep0.c does not.
If <config.h> is not included, any CONFIGs from the board header are defined.
The size of phys_addr_t depends on CONFIG_PHYS_64BIT as you see in:
#ifdef CONFIG_PHYS_64BIT typedef unsigned long long phys_addr_t; typedef unsigned long long phys_size_t; #else /* DMA addresses are 32-bits wide */ typedef unsigned long phys_addr_t; typedef unsigned long phys_size_t; #endif
So, phys_addr_t is 8 byte in gadget.c and phys_addr_t is 4 byte in ep0.c
My commit changed resource_size_t based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Thanks for detailed explanation. Correct config.h or common.h is needed for all source files ( particulary for all source or device drivers uses resource_size_t). I will update the git log and resend the patch.
Regards Ravi
On 07/22/2016 07:10 AM, Masahiro Yamada wrote:
Hi.
2016-07-21 21:29 GMT+09:00 B, Ravi ravibabu@ti.com:
Hi Marek
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
I will explain it more precisely.
Both gadget.c and ep.c include <linux/types.h>
See, include/linux/types.h is included from include/linux/kernel.h included from drivers/usb/dwc3/ep0.c
So, <linux/types.h> is not a problem.
The root cause of problem is: gadget.c include <config.h>, but ep0.c does not.
If <config.h> is not included, any CONFIGs from the board header are defined.
The size of phys_addr_t depends on CONFIG_PHYS_64BIT as you see in:
#ifdef CONFIG_PHYS_64BIT typedef unsigned long long phys_addr_t; typedef unsigned long long phys_size_t; #else /* DMA addresses are 32-bits wide */ typedef unsigned long phys_addr_t; typedef unsigned long phys_size_t; #endif
So, phys_addr_t is 8 byte in gadget.c and phys_addr_t is 4 byte in ep0.c
My commit changed resource_size_t based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Thanks for the sensible and understandable explanation.
On Fri, Jul 22, 2016 at 02:10:22PM +0900, Masahiro Yamada wrote:
Hi.
2016-07-21 21:29 GMT+09:00 B, Ravi ravibabu@ti.com:
Hi Marek
The crash at dwc3 driver observed due to offset misalignment of structure members across files causing wrong code generation and leads to crash, the issue is found during dfu test.
For instance, ther is is mismatch in code generation to access the address of structure member dwc->dep[0] in gadget.c and ep0.c. This leads to NULL pointer reference casuing the crash. The inclusion of common.h fixes the issue.
Please explain why this patch fixes the issue.
Ok I will explain, due to the commit[1] the resource_size_t size has increased to 8 bytes (64 bit), compared to earlier 32 bit (4bytes) and the definition is moved to includes/linux/types.h from asm.h. Due to this change the code generated in gadget.c is correct, due to inclusion of right header file (common.h, which includes linux/types.h). Whereas, the ep0.c does not includes common.h, hence size of resources_size_t is 4 bytes, causing wrong offset code generated for structure members which includes resource_size_t, which leads to pointing to wrong offset location causing the crash.
I will explain it more precisely.
Both gadget.c and ep.c include <linux/types.h>
See, include/linux/types.h is included from include/linux/kernel.h included from drivers/usb/dwc3/ep0.c
So, <linux/types.h> is not a problem.
The root cause of problem is: gadget.c include <config.h>, but ep0.c does not.
If <config.h> is not included, any CONFIGs from the board header are defined.
The size of phys_addr_t depends on CONFIG_PHYS_64BIT as you see in:
#ifdef CONFIG_PHYS_64BIT typedef unsigned long long phys_addr_t; typedef unsigned long long phys_size_t; #else /* DMA addresses are 32-bits wide */ typedef unsigned long phys_addr_t; typedef unsigned long phys_size_t; #endif
So, phys_addr_t is 8 byte in gadget.c and phys_addr_t is 4 byte in ep0.c
My commit changed resource_size_t based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Can we move PHYS_64BIT to Kconfig instead here please? This is the kind of thing we should be able to select based on SoC / board. Thanks!
Tom
based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Can we move PHYS_64BIT to Kconfig instead here please? This is the kind of thing we should be able to select based on SoC / board. Thanks!
Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out.
I have posted patch. Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based on Soc type
Regards Ravi
On Tue, Jul 26, 2016 at 12:59:14PM +0000, B, Ravi wrote:
Tom
based on phys_addr_t, so it triggered the problem for DWC3, which had already potentially existed.
CONFIGs in Kconfig are guaranteed to be defined for all files, but CONFIGs in board headers are not.
So we need to make sure to add #include <common.h> (or #include <config.h>) in each source file.
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Can we move PHYS_64BIT to Kconfig instead here please? This is the kind of thing we should be able to select based on SoC / board. Thanks!
Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out.
I have posted patch. Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based on Soc type
You missed that Masahiro posted one as well that converted all targets :( Can you please grab his and confirm that it fixes the problem as well? Thanks!
Hi Tom
So, your patch is doing a right thing.
I will issue my Reviewed-by when you update the git-log.
(Moving CONFIG_PHYS_64BIT is a right thing as well)
Can we move PHYS_64BIT to Kconfig instead here please? This is the kind of thing we should be able to select based on SoC / board. Thanks!
Yes, moving to PHYS_64BIT to Kconfig would be ideal fix. Thanks for pointing out.
I have posted patch. Commit: bac9e0: Kconfig: dra7x: Kconfig based PHYS_64BIT select based on Soc type
You missed that Masahiro posted one as well that converted all targets :( Can you please grab his and confirm that it fixes the problem as well? Thanks!
Oh yes, I missed it. Just reviewed it's the similar change and tested , worked as well. Thanks.
Regards Ravi
participants (5)
-
B, Ravi -
Marek Vasut -
Masahiro Yamada -
Ravi Babu
-
Tom Rini