[PATCH 0/3] efi_test: fix Coccinelle warning and CoverityScan issues

[PATCH 0/3] efi_test: fix Coccinelle warning and CoverityScan issues

[Ivan Hu]

Fix the memdup_user warning found by Cocinelle, and some minor uninitialized
scalar variable issues found by CoverityScan. 

Ivan Hu (3):
  efi/efi_test: use memdup_user() as a cleanup
  efi/efi_test: fix the uninitialized value datasize
  efi/efi_test: fix the uninitialized value rv

 drivers/firmware/efi/test/efi_test.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup

[Ivan Hu]

Cleanup the warning,
drivers/firmware/efi/test/efi_test.c:269:8-15: WARNING opportunity for
memdup_user

Use memdup_user rather than duplicating its implementation
This is a little bit restricted to reduce false positives

Generated by: coccinelle/api/memdup_user.coc

Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/firmware/efi/test/efi_test.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
index f61bb52..5602c46 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -266,14 +266,10 @@ static long efi_runtime_set_variable(unsigned long arg)
 			return rv;
 	}
 
-	data = kmalloc(setvariable.data_size, GFP_KERNEL);
-	if (!data) {
+	data = memdup_user(setvariable.data, setvariable.data_size);
+	if (IS_ERR(data)) {
 		kfree(name);
-		return -ENOMEM;
-	}
-	if (copy_from_user(data, setvariable.data, setvariable.data_size)) {
-		rv = -EFAULT;
-		goto out;
+		return PTR_ERR(data);
 	}
 
 	status = efi.set_variable(name, &vendor_guid,
-- 
1.9.1

Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup

[Matt Fleming]

On Mon, 26 Sep, at 11:14:49AM, Ivan Hu wrote:
> Cleanup the warning,
> drivers/firmware/efi/test/efi_test.c:269:8-15: WARNING opportunity for
> memdup_user
> 
> Use memdup_user rather than duplicating its implementation
> This is a little bit restricted to reduce false positives
 
I don't understand this sentence. What restriction are you talking
about?

> Generated by: coccinelle/api/memdup_user.coc
> 
> Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  drivers/firmware/efi/test/efi_test.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
> index f61bb52..5602c46 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -266,14 +266,10 @@ static long efi_runtime_set_variable(unsigned long arg)
>  			return rv;
>  	}
>  
> -	data = kmalloc(setvariable.data_size, GFP_KERNEL);
> -	if (!data) {
> +	data = memdup_user(setvariable.data, setvariable.data_size);
> +	if (IS_ERR(data)) {
>  		kfree(name);
> -		return -ENOMEM;
> -	}
> -	if (copy_from_user(data, setvariable.data, setvariable.data_size)) {
> -		rv = -EFAULT;
> -		goto out;
> +		return PTR_ERR(data);
>  	}
>  
>  	status = efi.set_variable(name, &vendor_guid,
> -- 
> 1.9.1
> 

Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup

[ivanhu]

Hi Matt,

The warning message is from Kbuild test robot,

https://chromium.googlesource.com/linux-fpga-chameleon/+/fpga-chameleon-4.2/scripts/coccinelle/api/memdup_user.cocci, 


and  I believe it tries to persuade us to use memdup_user instead of 
kmalloc and copy_form_user,

With this new function, we don't have to write 'len' twice, which can 
lead to typos/mistakes.
It also produces smaller code and kernel text.

http://lkml.iu.edu/hypermail/linux/kernel/0903.0/02349.html


Cheers,

Ivan

On 2016年10月04日 04:17, Matt Fleming wrote:
> On Mon, 26 Sep, at 11:14:49AM, Ivan Hu wrote:
>> Cleanup the warning,
>> drivers/firmware/efi/test/efi_test.c:269:8-15: WARNING opportunity for
>> memdup_user
>>
>> Use memdup_user rather than duplicating its implementation
>> This is a little bit restricted to reduce false positives
>   
> I don't understand this sentence. What restriction are you talking
> about?
>
>> Generated by: coccinelle/api/memdup_user.coc
>>
>> Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>>   drivers/firmware/efi/test/efi_test.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
>> index f61bb52..5602c46 100644
>> --- a/drivers/firmware/efi/test/efi_test.c
>> +++ b/drivers/firmware/efi/test/efi_test.c
>> @@ -266,14 +266,10 @@ static long efi_runtime_set_variable(unsigned long arg)
>>   			return rv;
>>   	}
>>   
>> -	data = kmalloc(setvariable.data_size, GFP_KERNEL);
>> -	if (!data) {
>> +	data = memdup_user(setvariable.data, setvariable.data_size);
>> +	if (IS_ERR(data)) {
>>   		kfree(name);
>> -		return -ENOMEM;
>> -	}
>> -	if (copy_from_user(data, setvariable.data, setvariable.data_size)) {
>> -		rv = -EFAULT;
>> -		goto out;
>> +		return PTR_ERR(data);
>>   	}
>>   
>>   	status = efi.set_variable(name, &vendor_guid,
>> -- 
>> 1.9.1
>>

Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup

[Matt Fleming]

On Tue, 04 Oct, at 11:04:56AM, ivanhu wrote:
> Hi Matt,
> 
> The warning message is from Kbuild test robot,
> 
> https://chromium.googlesource.com/linux-fpga-chameleon/+/fpga-chameleon-4.2/scripts/coccinelle/api/memdup_user.cocci,
> 
> 
> and  I believe it tries to persuade us to use memdup_user instead of kmalloc
> and copy_form_user,
> 
> With this new function, we don't have to write 'len' twice, which can lead
> to typos/mistakes.
> It also produces smaller code and kernel text.
> 
> http://lkml.iu.edu/hypermail/linux/kernel/0903.0/02349.html

Right, I understand all that. My question was specifically about the
following sentence from your patch,

> This is a little bit restricted to reduce false positives

What restriction that reduces false positives are you referring to?

Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup

[ivanhu]

On 2016年10月05日 05:37, Matt Fleming wrote:
> On Tue, 04 Oct, at 11:04:56AM, ivanhu wrote:
>> Hi Matt,
>>
>> The warning message is from Kbuild test robot,
>>
>> https://chromium.googlesource.com/linux-fpga-chameleon/+/fpga-chameleon-4.2/scripts/coccinelle/api/memdup_user.cocci,
>>
>>
>> and  I believe it tries to persuade us to use memdup_user instead of kmalloc
>> and copy_form_user,
>>
>> With this new function, we don't have to write 'len' twice, which can lead
>> to typos/mistakes.
>> It also produces smaller code and kernel text.
>>
>> http://lkml.iu.edu/hypermail/linux/kernel/0903.0/02349.html
>
> Right, I understand all that. My question was specifically about the
> following sentence from your patch,
>
>> This is a little bit restricted to reduce false positives
>
> What restriction that reduces false positives are you referring to?

I believe that author means that, with this new function, we don't have 
to write 'len' twice, which can lead to typos/mistakes ....

Let me remove the ambiguous comments and send out the patch again.

Ivan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/3] efi/efi_test: fix the uninitialized value datasize

[Ivan Hu]

Fix the minor issue found by CoverityScan
CID 1358931 (#1 of 1): Uninitialized scalar variable (UNINIT)9.
uninit_use: Using uninitialized value datasize.
199        prev_datasize = datasize;
200        status = efi.get_variable(name, vd, at, dz, data);

Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/firmware/efi/test/efi_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
index 5602c46..87394f0 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -156,7 +156,7 @@ static long efi_runtime_get_variable(unsigned long arg)
 {
 	struct efi_getvariable __user *getvariable_user;
 	struct efi_getvariable getvariable;
-	unsigned long datasize, prev_datasize, *dz;
+	unsigned long datasize = 0, prev_datasize, *dz;
 	efi_guid_t vendor_guid, *vd = NULL;
 	efi_status_t status;
 	efi_char16_t *name = NULL;
-- 
1.9.1

Re: [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize

[Matt Fleming]

On Mon, 26 Sep, at 11:14:50AM, Ivan Hu wrote:
> Fix the minor issue found by CoverityScan
> CID 1358931 (#1 of 1): Uninitialized scalar variable (UNINIT)9.
> uninit_use: Using uninitialized value datasize.
> 199        prev_datasize = datasize;
> 200        status = efi.get_variable(name, vd, at, dz, data);
> 
> Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  drivers/firmware/efi/test/efi_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
> index 5602c46..87394f0 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -156,7 +156,7 @@ static long efi_runtime_get_variable(unsigned long arg)
>  {
>  	struct efi_getvariable __user *getvariable_user;
>  	struct efi_getvariable getvariable;
> -	unsigned long datasize, prev_datasize, *dz;
> +	unsigned long datasize = 0, prev_datasize, *dz;
>  	efi_guid_t vendor_guid, *vd = NULL;
>  	efi_status_t status;
>  	efi_char16_t *name = NULL;

Thanks, applied to 'next'.

[PATCH 3/3] efi/efi_test: fix the uninitialized value rv

[Ivan Hu]

Fix the minor issue found by CoverityScan
520        kfree(name);
CID 1358932 (#1 of 1): Uninitialized scalar variable (UNINIT)17.
uninit_use: Using uninitialized value rv.
521        return rv;
522}

Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/firmware/efi/test/efi_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
index 87394f0..39302e6 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -425,7 +425,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
 	efi_guid_t *vd = NULL;
 	efi_guid_t vendor_guid;
 	efi_char16_t *name = NULL;
-	int rv;
+	int rv = 0;
 
 	getnextvariablename_user = (struct efi_getnextvariablename __user *)arg;
 
-- 
1.9.1

Re: [PATCH 3/3] efi/efi_test: fix the uninitialized value rv

[Matt Fleming]

On Mon, 26 Sep, at 11:14:51AM, Ivan Hu wrote:
> Fix the minor issue found by CoverityScan
> 520        kfree(name);
> CID 1358932 (#1 of 1): Uninitialized scalar variable (UNINIT)17.
> uninit_use: Using uninitialized value rv.
> 521        return rv;
> 522}
> 
> Signed-off-by: Ivan Hu <ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  drivers/firmware/efi/test/efi_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
> index 87394f0..39302e6 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -425,7 +425,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>  	efi_guid_t *vd = NULL;
>  	efi_guid_t vendor_guid;
>  	efi_char16_t *name = NULL;
> -	int rv;
> +	int rv = 0;
>  
>  	getnextvariablename_user = (struct efi_getnextvariablename __user *)arg;
>  

Thanks, applied to 'next'.