[PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-23 Thread Lee Jones 
In its current state, the ux500-musb driver uses platform data pointers
blindly with no prior checking. If no platform data pointer is passed
this will Oops the kernel. In this patch we ensure platform data and
board data are present prior to using them.

Cc: Felipe Balbi 
Cc: linux-usb@vger.kernel.org
Signed-off-by: Lee Jones 
---
 drivers/usb/musb/ux500_dma.c |   11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/ux500_dma.c b/drivers/usb/musb/ux500_dma.c
index c75e07a..02b4a6e 100644
--- a/drivers/usb/musb/ux500_dma.c
+++ b/drivers/usb/musb/ux500_dma.c
@@ -287,7 +287,7 @@ static int ux500_dma_controller_start(struct dma_controller 
*c)
struct musb *musb = controller->private_data;
struct device *dev = musb->controller;
struct musb_hdrc_platform_data *plat = dev->platform_data;
-   struct ux500_musb_board_data *data = plat->board_data;
+   struct ux500_musb_board_data *data;
struct dma_channel *dma_channel = NULL;
u32 ch_num;
u8 dir;
@@ -297,14 +297,19 @@ static int ux500_dma_controller_start(struct 
dma_controller *c)
struct ux500_dma_channel *channel_array;
dma_cap_mask_t mask;
 
+   if (!plat) {
+   dev_err(musb->controller, "No platform data\n");
+   return -EINVAL;
+   }
 
+   data = plat->board_data;
 
dma_cap_zero(mask);
dma_cap_set(DMA_SLAVE, mask);
 
/* Prepare the loop for RX channels */
channel_array = controller->rx_channel;
-   param_array = data->dma_rx_param_array;
+   param_array = (data) ? data->dma_rx_param_array : NULL;
 
for (dir = 0; dir < 2; dir++) {
for (ch_num = 0;
@@ -337,7 +342,7 @@ static int ux500_dma_controller_start(struct dma_controller 
*c)
 
/* Prepare the loop for TX channels */
channel_array = controller->tx_channel;
-   param_array = data->dma_tx_param_array;
+   param_array = (data) ? data->dma_tx_param_array : NULL;
is_tx = 1;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-23 Thread Sergei Shtylyov 

Hello.

On 04/23/2013 07:03 PM, Lee Jones wrote:


In its current state, the ux500-musb driver uses platform data pointers
blindly with no prior checking. If no platform data pointer is passed
this will Oops the kernel. In this patch we ensure platform data and
board data are present prior to using them.

Cc: Felipe Balbi 
Cc: linux-usb@vger.kernel.org
Signed-off-by: Lee Jones 
---
  drivers/usb/musb/ux500_dma.c |   11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/ux500_dma.c b/drivers/usb/musb/ux500_dma.c
index c75e07a..02b4a6e 100644
--- a/drivers/usb/musb/ux500_dma.c
+++ b/drivers/usb/musb/ux500_dma.c
@@ -287,7 +287,7 @@ static int ux500_dma_controller_start(struct dma_controller 
*c)
struct musb *musb = controller->private_data;
struct device *dev = musb->controller;
struct musb_hdrc_platform_data *plat = dev->platform_data;
-   struct ux500_musb_board_data *data = plat->board_data;
+   struct ux500_musb_board_data *data;
struct dma_channel *dma_channel = NULL;
u32 ch_num;
u8 dir;
@@ -297,14 +297,19 @@ static int ux500_dma_controller_start(struct 
dma_controller *c)


[...]

  
  	/* Prepare the loop for RX channels */

channel_array = controller->rx_channel;
-   param_array = data->dma_rx_param_array;
+   param_array = (data) ? data->dma_rx_param_array : NULL;


   Why enclose a simple variable in parens?

  
  	for (dir = 0; dir < 2; dir++) {

for (ch_num = 0;
@@ -337,7 +342,7 @@ static int ux500_dma_controller_start(struct dma_controller 
*c)
  
  		/* Prepare the loop for TX channels */

channel_array = controller->tx_channel;
-   param_array = data->dma_tx_param_array;
+   param_array = (data) ? data->dma_tx_param_array : NULL;


   Again, why?

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-23 Thread Lee Jones 
Hi Sergei,

> > struct musb_hdrc_platform_data *plat = dev->platform_data;
> >-struct ux500_musb_board_data *data = plat->board_data;
> >+struct ux500_musb_board_data *data;

> >-param_array = data->dma_rx_param_array;
> >+param_array = (data) ? data->dma_rx_param_array : NULL;
> 
>Why enclose a simple variable in parens?

Because 'data' is a pointer, so it contains a memory location, but if
'plat->board_data' is NULL, then 'data' will be NULL (essentially
memory location 0x).

So if we were to read-in to 'struct ux500_musb_board_data *data', by
index 'dma_rx_param_array', which I believe is '0' in this case:

struct ux500_musb_board_data {
void**dma_rx_param_array;
void**dma_tx_param_array;
bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
};

... then we're saying take the data from this memory location:

param_array = *((0x)->(0x0));

Which will cause a kernel Oops, due to the fact that address 0x0 isn't
allocated to us, so you get something like:

"Unable to handle kernel NULL pointer dereference at virtual address "

Hope that helps.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-24 Thread Sergei Shtylyov 

Hello.

On 24-04-2013 10:53, Lee Jones wrote:


struct musb_hdrc_platform_data *plat = dev->platform_data;
-   struct ux500_musb_board_data *data = plat->board_data;
+   struct ux500_musb_board_data *data;



-   param_array = data->dma_rx_param_array;
+   param_array = (data) ? data->dma_rx_param_array : NULL;



Why enclose a simple variable in parens?



Because 'data' is a pointer, so it contains a memory location,


   Pointer points to memory location, not contains it.


but if
'plat->board_data' is NULL, then 'data' will be NULL (essentially
memory location 0x).


   So what?


So if we were to read-in to 'struct ux500_musb_board_data *data', by
index 'dma_rx_param_array', which I believe is '0' in this case:



struct ux500_musb_board_data {
 void**dma_rx_param_array;
 void**dma_tx_param_array;
 bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
};



... then we're saying take the data from this memory location:



param_array = *((0x)->(0x0));



Which will cause a kernel Oops, due to the fact that address 0x0 isn't
allocated to us, so you get something like:



"Unable to handle kernel NULL pointer dereference at virtual address "


   We're not dereferencing 'data', so I completely fail to follow you.


Hope that helps.


   Not at all.


Kind regards,
Lee


WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-24 Thread Felipe Balbi 
On Wed, Apr 24, 2013 at 06:00:28PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 24-04-2013 10:53, Lee Jones wrote:
> 
> >>>   struct musb_hdrc_platform_data *plat = dev->platform_data;
> >>>-  struct ux500_musb_board_data *data = plat->board_data;
> >>>+  struct ux500_musb_board_data *data;
> 
> >>>-  param_array = data->dma_rx_param_array;
> >>>+  param_array = (data) ? data->dma_rx_param_array : NULL;
> 
> >>Why enclose a simple variable in parens?
> 
> >Because 'data' is a pointer, so it contains a memory location,

heh, I don't think you fully understood Sergei's comment. He's asking
why you used:

param_array = (data) ? data->dma_rx_param_array : NULL;

instead of:

param_array = data ? data->dma_rx_param_array : NULL;

He's saying, correctly so, that the parens around data is unnecessary.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 04/10] usb: musb: ux500: harden checks for platform data

2013-04-24 Thread Arnd Bergmann 
On Wednesday 24 April 2013, Lee Jones wrote:
> Yeah, I agree, although does it make a difference?
> 
> Is it evaluated a second time, or does it take up extra cycles by being
> enclosed in parentheses?
> 
> Or is this just a coding style thing?

Just coding style. I agree you should have no parentheses there, but it
does not change the compiled code.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html