Re: [PATCH] thunderbolt: property: fix a buffer overflow and a missing check

[Mukesh Ojha]

On 3/25/2019 4:19 AM, Kangjie Lu wrote:

First, no memory is allocated for "property->value.text"; the
following strcpy will lead to a buffer overflow.


Fix the commit text as there is no  overflow.

only the check and resource cleanp is the fix.



Second, no check is enforced for the return value of kzalloc,
which may lead to NULL-pointer dereference.

The patch fixes the two issues.

Signed-off-by: Kangjie Lu 
Otherwise looks good.

Reviewed-by: Mukesh Ojha 

-Mukesh


---
  drivers/thunderbolt/property.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index 841314deb446..d5b0cdb8f0b1 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -587,7 +587,12 @@ int tb_property_add_text(struct tb_property_dir *parent, 
const char *key,
return -ENOMEM;
  
  	property->length = size / 4;

-   property->value.data = kzalloc(size, GFP_KERNEL);
+   property->value.text = kzalloc(size, GFP_KERNEL);
+   if (!property->value.text) {
+   kfree(property);
+   return -ENOMEM;
+   }
+
strcpy(property->value.text, text);
  
  	list_add_tail(>list, >properties);

Re: [PATCH] thunderbolt: property: fix a buffer overflow and a missing check

[Mika Westerberg]

On Sun, Mar 24, 2019 at 05:49:16PM -0500, Kangjie Lu wrote:
> First, no memory is allocated for "property->value.text"; the
> following strcpy will lead to a buffer overflow.

It is actually member of union so assigning via value.txt or value.data
is the same.

So no buffer overflow.

> Second, no check is enforced for the return value of kzalloc,
> which may lead to NULL-pointer dereference.

Yes, this is valid.

Can you fix the changelog accordingly and resend?

> 
> The patch fixes the two issues.
> 
> Signed-off-by: Kangjie Lu 
> ---
>  drivers/thunderbolt/property.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
> index 841314deb446..d5b0cdb8f0b1 100644
> --- a/drivers/thunderbolt/property.c
> +++ b/drivers/thunderbolt/property.c
> @@ -587,7 +587,12 @@ int tb_property_add_text(struct tb_property_dir *parent, 
> const char *key,
>   return -ENOMEM;
>  
>   property->length = size / 4;
> - property->value.data = kzalloc(size, GFP_KERNEL);
> + property->value.text = kzalloc(size, GFP_KERNEL);
> + if (!property->value.text) {
> + kfree(property);
> + return -ENOMEM;
> + }
> +
>   strcpy(property->value.text, text);
>  
>   list_add_tail(>list, >properties);
> -- 
> 2.17.1

[PATCH] thunderbolt: property: fix a buffer overflow and a missing check

[Kangjie Lu]

First, no memory is allocated for "property->value.text"; the
following strcpy will lead to a buffer overflow.

Second, no check is enforced for the return value of kzalloc,
which may lead to NULL-pointer dereference.

The patch fixes the two issues.

Signed-off-by: Kangjie Lu 
---
 drivers/thunderbolt/property.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/property.c b/drivers/thunderbolt/property.c
index 841314deb446..d5b0cdb8f0b1 100644
--- a/drivers/thunderbolt/property.c
+++ b/drivers/thunderbolt/property.c
@@ -587,7 +587,12 @@ int tb_property_add_text(struct tb_property_dir *parent, 
const char *key,
return -ENOMEM;
 
property->length = size / 4;
-   property->value.data = kzalloc(size, GFP_KERNEL);
+   property->value.text = kzalloc(size, GFP_KERNEL);
+   if (!property->value.text) {
+   kfree(property);
+   return -ENOMEM;
+   }
+
strcpy(property->value.text, text);
 
list_add_tail(>list, >properties);
-- 
2.17.1