staging: netlogic: Return zero pointer after failed kmalloc

PATCH In review Laura Garcia Liebana 2 participants 6 messages Feb 18, 2016 – Feb 24, 2016 Staging

Share & tools

Laura Garcia Liebana

author

2016-02-18 17:38 UTC #0 lore raw

PATCH 1/1 staging: netlogic: Return zero pointer after failed kmalloc

Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.

Signed-off-by Laura Garcia Liebana <[email protected]>

---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/staging/netlogic/xlr_net.c +3 -1

index 98e74d7..0015847 100644
@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;
+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);

Signature · 1 line

2.7.0

Julia Lawall

2016-02-23 20:42 UTC #1 lore raw

Quoted 14 lines · Laura Garcia Liebana

Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.
Signed-off-by Laura Garcia Liebana <[email protected]>
---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/netlogic/xlr_net.c +0 -0
index 98e74d7..0015847 100644

drivers/staging/netlogic/xlr_net.c static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0, · line 442

Quoted 7 lines

drivers/staging/netlogic/xlr_net.c +2 -1

@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;

Why did you choose this as the return value, rather than NULL? Is there a way that the first argument to kmalloc can be 0? Also, the containing function, xlr_config_spill, is called in a number of places where the results is stored in a structure field with no error checking. Have you traced through the code to see if it is appropriate to store and invalid pointer in these fields? At the moment, the crash will at least happen near the location of the faulty assignment. It could get harder to debug, if the bad value will propagate through structure fields to other parts of the driver.

julia

Quoted 14 lines

drivers/staging/netlogic/xlr_net.c +1 -0

+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);

Signature · 8 lines

2.7.0

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160218173839.GA9157%40sonyv.
For more options, visit https://groups.google.com/d/optout.

Laura Garcia

author

2016-02-23 22:36 UTC #2 lore raw

Quoted 45 lines · Julia Lawall

Quoted 14 lines · Laura Garcia Liebana
Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.
Signed-off-by Laura Garcia Liebana <[email protected]>
---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/netlogic/xlr_net.c +0 -0
index 98e74d7..0015847 100644
drivers/staging/netlogic/xlr_net.c static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0, · line 442
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +2 -1
@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;
Why did you choose this as the return value, rather than NULL? Is there a way that the first argument to kmalloc can be 0? Also, the containing function, xlr_config_spill, is called in a number of places where the results is stored in a structure field with no error checking. Have you traced through the code to see if it is appropriate to store and invalid pointer in these fields? At the moment, the crash will at least happen near the location of the faulty assignment. It could get harder to debug, if the bad value will propagate through structure fields to other parts of the driver.
julia
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +1 -0
+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);
Signature · 1 line
2.7.0

Quoted 7 lines

Signature · 5 lines

You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160218173839.GA9157%40sonyv.
For more options, visit https://groups.google.com/d/optout.

I guessed that it was the best way to return an invalid pointer after a kmalloc. Doesn't seem that the structure where the function return is stored is called anymore. So I believe that returning a NULL will be safe in this case.

Julia Lawall

2016-02-24 09:23 UTC #3 lore raw

Quoted 46 lines · Laura Garcia

Quoted 45 lines · Julia Lawall
Quoted 14 lines · Laura Garcia Liebana
Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.
Signed-off-by Laura Garcia Liebana <[email protected]>
---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/netlogic/xlr_net.c +0 -0
index 98e74d7..0015847 100644
drivers/staging/netlogic/xlr_net.c static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0, · line 442
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +2 -1
@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;
Why did you choose this as the return value, rather than NULL? Is there a way that the first argument to kmalloc can be 0? Also, the containing function, xlr_config_spill, is called in a number of places where the results is stored in a structure field with no error checking. Have you traced through the code to see if it is appropriate to store and invalid pointer in these fields? At the moment, the crash will at least happen near the location of the faulty assignment. It could get harder to debug, if the bad value will propagate through structure fields to other parts of the driver.
julia
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +1 -0
+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);
Signature · 1 line
2.7.0

Quoted 12 lines

Quoted 7 lines
Quoted 7 lines
Signature · 5 lines
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160218173839.GA9157%40sonyv.
For more options, visit https://groups.google.com/d/optout.
I guessed that it was the best way to return an invalid pointer after a kmalloc. Doesn't seem that the structure where the function return is stored is called anymore. So I believe that returning a NULL will be safe in this case.

I'm not sure to understand the second sentence. Is the kmalloc useless? To my recollection, the result of the function is stored in several structure fields. Are these structure fields never used?

Also, when you reply to a comment, could you put your reply just under the comment, with blank lines around it? Otherwise, the reply is hard to find.

thanks,
julia

Quoted 8 lines

Signature · 6 lines

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160223223647.GA4772%40sonyv.
For more options, visit https://groups.google.com/d/optout.

Laura Garcia

author

2016-02-24 16:47 UTC #4 lore raw

Quoted 63 lines · Julia Lawall

Quoted 46 lines · Laura Garcia
Quoted 45 lines · Julia Lawall
Quoted 14 lines · Laura Garcia Liebana
Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.
Signed-off-by Laura Garcia Liebana <[email protected]>
---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/netlogic/xlr_net.c +0 -0
index 98e74d7..0015847 100644
drivers/staging/netlogic/xlr_net.c static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0, · line 442
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +2 -1
@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;
Why did you choose this as the return value, rather than NULL? Is there a way that the first argument to kmalloc can be 0? Also, the containing function, xlr_config_spill, is called in a number of places where the results is stored in a structure field with no error checking. Have you traced through the code to see if it is appropriate to store and invalid pointer in these fields? At the moment, the crash will at least happen near the location of the faulty assignment. It could get harder to debug, if the bad value will propagate through structure fields to other parts of the driver.
julia
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +1 -0
+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);
Signature · 1 line
2.7.0
Quoted 12 lines
Quoted 7 lines
Quoted 7 lines
Signature · 5 lines
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160218173839.GA9157%40sonyv.
For more options, visit https://groups.google.com/d/optout.
I guessed that it was the best way to return an invalid pointer after a kmalloc. Doesn't seem that the structure where the function return is stored is called anymore. So I believe that returning a NULL will be safe in this case.

Quoted 4 lines

I'm not sure to understand the second sentence. Is the kmalloc useless? To my recollection, the result of the function is stored in several structure fields. Are these structure fields never used?

Exactly, these structure fields seem that are never used.

Quoted 4 lines

Also, when you reply to a comment, could you put your reply just under the comment, with blank lines around it? Otherwise, the reply is hard to find.

Ok, got it.

thanks.

Quoted 3 lines

thanks,
julia

Julia Lawall

2016-02-24 16:55 UTC #5 lore raw

Quoted 64 lines · Laura Garcia

Quoted 63 lines · Julia Lawall
Quoted 58 lines · Laura Garcia
Quoted 45 lines · Julia Lawall
Quoted 14 lines · Laura Garcia Liebana
Return a ZERO_SIZE_PTR in the xlr_config_spill function if the kmalloc returns an invalid value. This change prevents a possible segmentation fault as the invalid pointer is fed into PTR_ALIGN macro.
Signed-off-by Laura Garcia Liebana <[email protected]>
---
 drivers/staging/netlogic/xlr_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/netlogic/xlr_net.c +0 -0
index 98e74d7..0015847 100644
drivers/staging/netlogic/xlr_net.c static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0, · line 442
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +2 -1
@@ -437,8 +437,10 @@ static void *xlr_config_spill(struct xlr_net_priv *priv, int reg_start_0,
 	base = priv->base_addr;
 	spill_size = size;
 	spill = kmalloc(spill_size + SMP_CACHE_BYTES, GFP_ATOMIC);
-	if (!spill)
+	if (!spill) {
 		pr_err("Unable to allocate memory for spill area!\n");
+		return ZERO_SIZE_PTR;
Why did you choose this as the return value, rather than NULL? Is there a way that the first argument to kmalloc can be 0? Also, the containing function, xlr_config_spill, is called in a number of places where the results is stored in a structure field with no error checking. Have you traced through the code to see if it is appropriate to store and invalid pointer in these fields? At the moment, the crash will at least happen near the location of the faulty assignment. It could get harder to debug, if the bad value will propagate through structure fields to other parts of the driver.
julia
Quoted 7 lines
drivers/staging/netlogic/xlr_net.c +1 -1
+	}
 
 	spill = PTR_ALIGN(spill, SMP_CACHE_BYTES);
 	phys_addr = virt_to_phys(spill);
--
2.7.0
Quoted 7 lines
Quoted 7 lines
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160218173839.GA9157%40sonyv.
For more options, visit https://groups.google.com/d/optout.
I guessed that it was the best way to return an invalid pointer after a kmalloc. Doesn't seem that the structure where the function return is stored is called anymore. So I believe that returning a NULL will be safe in this case.

Quoted 6 lines

Quoted 4 lines
I'm not sure to understand the second sentence. Is the kmalloc useless? To my recollection, the result of the function is stored in several structure fields. Are these structure fields never used?
Exactly, these structure fields seem that are never used.

Then maybe the whole function is useless? It would seem to be something to study carefully, and then perhaps ask the maintainers for confirmation.

julia

Quoted 15 lines

Quoted 4 lines
Also, when you reply to a comment, could you put your reply just under the comment, with blank lines around it? Otherwise, the reply is hard to find.
Ok, got it.
thanks.
Quoted 3 lines
thanks,
julia