[U-Boot] [PATCH] usb: dwc3: fixes crash in dwc3 driver due to types size mismatch

[Tom Rini]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/3a8a14c6/attachment.sig>