[PATCH] staging: vt6655: check for memory allocation failures

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] staging: vt6655: check for memory allocation failures
       [not found] <CGME20180328063144epcas1p305216b582520b884b09c93a7ad591660@epcas1p3.samsung.com>
@ 2018-03-28  6:31   ` Ji-Hun Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Ji-Hun Kim @ 2018-03-28  6:31 UTC (permalink / raw)
  To: gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, ji_hun.kim, linux-kernel,
	julia.lawall, baijiaju1990, santhameena13

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
---
 drivers/staging/vt6655/device_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index fbc4bc6..5d0ba94 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD0Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->rd_info))
+			return;
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD1Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->rd_info))
+			return;
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD0Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->td_info))
+			return;
 		desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD1Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->td_info))
+			return;
 		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
 
-- 
1.9.1


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

* [PATCH] staging: vt6655: check for memory allocation failures
@ 2018-03-28  6:31   ` Ji-Hun Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Ji-Hun Kim @ 2018-03-28  6:31 UTC (permalink / raw)
  To: gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, ji_hun.kim, linux-kernel,
	julia.lawall, baijiaju1990, santhameena13

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
---
 drivers/staging/vt6655/device_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index fbc4bc6..5d0ba94 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD0Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->rd_info))
+			return;
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD1Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->rd_info))
+			return;
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD0Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->td_info))
+			return;
 		desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD1Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (WARN_ON(!desc->td_info))
+			return;
 		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
 
-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
  2018-03-28  6:31   ` Ji-Hun Kim
@ 2018-03-28  8:33     ` Greg KH
  -1 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2018-03-28  8:33 UTC (permalink / raw)
  To: Ji-Hun Kim
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	baijiaju1990, forest, santhameena13

On Wed, Mar 28, 2018 at 03:31:31PM +0900, Ji-Hun Kim wrote:
> There are no null pointer checking on rd_info and td_info values which
> are allocated by kzalloc. It has potential null pointer dereferencing
> issues. Add return when allocation is failed.
> 
> Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
> ---
>  drivers/staging/vt6655/device_main.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index fbc4bc6..5d0ba94 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
>  	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>  		desc = &priv->aRD0Ring[i];
>  		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;

Eeek, no, this crashes any machine that runs with "panic on warn".  You
don't crash if you can not allocate any memory, you recover and move on.
You don't even need to print anything out, as the call itself will do
that if this happens.

So this patch isn't ok at all, sorry.

greg k-h

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
@ 2018-03-28  8:33     ` Greg KH
  0 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2018-03-28  8:33 UTC (permalink / raw)
  To: Ji-Hun Kim
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	baijiaju1990, forest, santhameena13

On Wed, Mar 28, 2018 at 03:31:31PM +0900, Ji-Hun Kim wrote:
> There are no null pointer checking on rd_info and td_info values which
> are allocated by kzalloc. It has potential null pointer dereferencing
> issues. Add return when allocation is failed.
> 
> Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
> ---
>  drivers/staging/vt6655/device_main.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index fbc4bc6..5d0ba94 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
>  	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>  		desc = &priv->aRD0Ring[i];
>  		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;

Eeek, no, this crashes any machine that runs with "panic on warn".  You
don't crash if you can not allocate any memory, you recover and move on.
You don't even need to print anything out, as the call itself will do
that if this happens.

So this patch isn't ok at all, sorry.

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
  2018-03-28  6:31   ` Ji-Hun Kim
@ 2018-03-28  9:55     ` Jia-Ju Bai
  -1 siblings, 0 replies; 8+ messages in thread
From: Jia-Ju Bai @ 2018-03-28  9:55 UTC (permalink / raw)
  To: Ji-Hun Kim, gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	santhameena13



On 2018/3/28 14:31, Ji-Hun Kim wrote:
> There are no null pointer checking on rd_info and td_info values which
> are allocated by kzalloc. It has potential null pointer dereferencing
> issues. Add return when allocation is failed.
>
> Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
> ---
>   drivers/staging/vt6655/device_main.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index fbc4bc6..5d0ba94 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
>   	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>   		desc = &priv->aRD0Ring[i];
>   		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;
>   		if (!device_alloc_rx_buf(priv, desc))
>   			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>   
> @@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
>   	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>   		desc = &priv->aRD1Ring[i];
>   		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;
>   		if (!device_alloc_rx_buf(priv, desc))
>   			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>   
> @@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
>   	     i++, curr += sizeof(struct vnt_tx_desc)) {
>   		desc = &priv->apTD0Rings[i];
>   		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->td_info))
> +			return;
>   		desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
>   		desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
>   
> @@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
>   	     i++, curr += sizeof(struct vnt_tx_desc)) {
>   		desc = &priv->apTD1Rings[i];
>   		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->td_info))
> +			return;
>   		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
>   		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
>   

I think the bugs you found are right.
But your patch is not correct, because it is dangerous to return directly.
I think you should return an error and then implement error handling 
code for these functions.


Best wishes,
Jia-Ju Bai

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
@ 2018-03-28  9:55     ` Jia-Ju Bai
  0 siblings, 0 replies; 8+ messages in thread
From: Jia-Ju Bai @ 2018-03-28  9:55 UTC (permalink / raw)
  To: Ji-Hun Kim, gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	santhameena13



On 2018/3/28 14:31, Ji-Hun Kim wrote:
> There are no null pointer checking on rd_info and td_info values which
> are allocated by kzalloc. It has potential null pointer dereferencing
> issues. Add return when allocation is failed.
>
> Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
> ---
>   drivers/staging/vt6655/device_main.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index fbc4bc6..5d0ba94 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
>   	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>   		desc = &priv->aRD0Ring[i];
>   		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;
>   		if (!device_alloc_rx_buf(priv, desc))
>   			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>   
> @@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
>   	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>   		desc = &priv->aRD1Ring[i];
>   		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->rd_info))
> +			return;
>   		if (!device_alloc_rx_buf(priv, desc))
>   			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>   
> @@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
>   	     i++, curr += sizeof(struct vnt_tx_desc)) {
>   		desc = &priv->apTD0Rings[i];
>   		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->td_info))
> +			return;
>   		desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
>   		desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
>   
> @@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
>   	     i++, curr += sizeof(struct vnt_tx_desc)) {
>   		desc = &priv->apTD1Rings[i];
>   		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> -
> +		if (WARN_ON(!desc->td_info))
> +			return;
>   		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
>   		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
>   

I think the bugs you found are right.
But your patch is not correct, because it is dangerous to return directly.
I think you should return an error and then implement error handling 
code for these functions.


Best wishes,
Jia-Ju Bai
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
  2018-03-28  9:55     ` Jia-Ju Bai
@ 2018-03-29  1:56       ` Ji-Hun Kim
  -1 siblings, 0 replies; 8+ messages in thread
From: Ji-Hun Kim @ 2018-03-29  1:56 UTC (permalink / raw)
  To: baijiaju1990, gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	santhameena13

On Wed, Mar 28, 2018 at 05:55:57PM +0800, Jia-Ju Bai wrote:
> >@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
> >  	     i++, curr += sizeof(struct vnt_tx_desc)) {
> >  		desc = &priv->apTD1Rings[i];
> >  		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> >-
> >+		if (WARN_ON(!desc->td_info))
> >+			return;
> >  		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
> >  		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
> 
> I think the bugs you found are right.
> But your patch is not correct, because it is dangerous to return directly.
> I think you should return an error and then implement error handling
> code for these functions.
> 
Yes, it needs to free previous allocated values in the for loop. Directly
return could make memory leaks. I am going to make patch v2. 

- Delete WARN_ON which could make crashes on some machines.
- Add freeing sequences for previously allocated memory when kzalloc()
  failed instead of returning directly.

Does these changes would be fine on this bug?

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

* Re: [PATCH] staging: vt6655: check for memory allocation failures
@ 2018-03-29  1:56       ` Ji-Hun Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Ji-Hun Kim @ 2018-03-29  1:56 UTC (permalink / raw)
  To: baijiaju1990, gregkh, forest
  Cc: devel, y.k.oh, kernel-janitors, linux-kernel, julia.lawall,
	santhameena13

On Wed, Mar 28, 2018 at 05:55:57PM +0800, Jia-Ju Bai wrote:
> >@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
> >  	     i++, curr += sizeof(struct vnt_tx_desc)) {
> >  		desc = &priv->apTD1Rings[i];
> >  		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> >-
> >+		if (WARN_ON(!desc->td_info))
> >+			return;
> >  		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
> >  		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
> 
> I think the bugs you found are right.
> But your patch is not correct, because it is dangerous to return directly.
> I think you should return an error and then implement error handling
> code for these functions.
> 
Yes, it needs to free previous allocated values in the for loop. Directly
return could make memory leaks. I am going to make patch v2. 

- Delete WARN_ON which could make crashes on some machines.
- Add freeing sequences for previously allocated memory when kzalloc()
  failed instead of returning directly.

Does these changes would be fine on this bug?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

end of thread, other threads:[~2018-03-29  1:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20180328063144epcas1p305216b582520b884b09c93a7ad591660@epcas1p3.samsung.com>
2018-03-28  6:31 ` [PATCH] staging: vt6655: check for memory allocation failures Ji-Hun Kim
2018-03-28  6:31   ` Ji-Hun Kim
2018-03-28  8:33   ` Greg KH
2018-03-28  8:33     ` Greg KH
2018-03-28  9:55   ` Jia-Ju Bai
2018-03-28  9:55     ` Jia-Ju Bai
2018-03-29  1:56     ` Ji-Hun Kim
2018-03-29  1:56       ` Ji-Hun Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.