* [PATCH 0/3] efi_test: fix Coccinelle warning and CoverityScan issues
@ 2016-09-26 3:14 Ivan Hu
[not found] ` <1474859691-8574-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Ivan Hu @ 2016-09-26 3:14 UTC (permalink / raw)
To: matt-mF/unelCI9GS6iBeEJttW/XRex20P6io, linux-efi-u79uwXL29TY76Z2rM5mHXA
Cc: 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup
[not found] ` <1474859691-8574-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2016-09-26 3:14 ` Ivan Hu
[not found] ` <1474859691-8574-2-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-09-26 3:14 ` [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize Ivan Hu
2016-09-26 3:14 ` [PATCH 3/3] efi/efi_test: fix the uninitialized value rv Ivan Hu
2 siblings, 1 reply; 10+ messages in thread
From: Ivan Hu @ 2016-09-26 3:14 UTC (permalink / raw)
To: matt-mF/unelCI9GS6iBeEJttW/XRex20P6io, linux-efi-u79uwXL29TY76Z2rM5mHXA
Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize
[not found] ` <1474859691-8574-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-09-26 3:14 ` [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup Ivan Hu
@ 2016-09-26 3:14 ` Ivan Hu
[not found] ` <1474859691-8574-3-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-09-26 3:14 ` [PATCH 3/3] efi/efi_test: fix the uninitialized value rv Ivan Hu
2 siblings, 1 reply; 10+ messages in thread
From: Ivan Hu @ 2016-09-26 3:14 UTC (permalink / raw)
To: matt-mF/unelCI9GS6iBeEJttW/XRex20P6io, linux-efi-u79uwXL29TY76Z2rM5mHXA
Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] efi/efi_test: fix the uninitialized value rv
[not found] ` <1474859691-8574-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-09-26 3:14 ` [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup Ivan Hu
2016-09-26 3:14 ` [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize Ivan Hu
@ 2016-09-26 3:14 ` Ivan Hu
[not found] ` <1474859691-8574-4-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2 siblings, 1 reply; 10+ messages in thread
From: Ivan Hu @ 2016-09-26 3:14 UTC (permalink / raw)
To: matt-mF/unelCI9GS6iBeEJttW/XRex20P6io, linux-efi-u79uwXL29TY76Z2rM5mHXA
Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup
[not found] ` <1474859691-8574-2-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2016-10-03 20:17 ` Matt Fleming
[not found] ` <20161003201718.GL16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Matt Fleming @ 2016-10-03 20:17 UTC (permalink / raw)
To: Ivan Hu; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize
[not found] ` <1474859691-8574-3-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2016-10-03 20:23 ` Matt Fleming
0 siblings, 0 replies; 10+ messages in thread
From: Matt Fleming @ 2016-10-03 20:23 UTC (permalink / raw)
To: Ivan Hu; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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'.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] efi/efi_test: fix the uninitialized value rv
[not found] ` <1474859691-8574-4-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2016-10-03 20:25 ` Matt Fleming
0 siblings, 0 replies; 10+ messages in thread
From: Matt Fleming @ 2016-10-03 20:25 UTC (permalink / raw)
To: Ivan Hu; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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'.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup
[not found] ` <20161003201718.GL16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
@ 2016-10-04 3:04 ` ivanhu
[not found] ` <3fb6d9fa-b554-509b-fc59-d75340017589-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: ivanhu @ 2016-10-04 3:04 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup
[not found] ` <3fb6d9fa-b554-509b-fc59-d75340017589-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2016-10-04 21:37 ` Matt Fleming
[not found] ` <20161004213743.GV16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Matt Fleming @ 2016-10-04 21:37 UTC (permalink / raw)
To: ivanhu; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup
[not found] ` <20161004213743.GV16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
@ 2016-10-05 1:31 ` ivanhu
0 siblings, 0 replies; 10+ messages in thread
From: ivanhu @ 2016-10-05 1:31 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA, Ard Biesheuvel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-05 1:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-26 3:14 [PATCH 0/3] efi_test: fix Coccinelle warning and CoverityScan issues Ivan Hu
[not found] ` <1474859691-8574-1-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-09-26 3:14 ` [PATCH 1/3] efi/efi_test: use memdup_user() as a cleanup Ivan Hu
[not found] ` <1474859691-8574-2-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-10-03 20:17 ` Matt Fleming
[not found] ` <20161003201718.GL16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-10-04 3:04 ` ivanhu
[not found] ` <3fb6d9fa-b554-509b-fc59-d75340017589-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-10-04 21:37 ` Matt Fleming
[not found] ` <20161004213743.GV16071-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-10-05 1:31 ` ivanhu
2016-09-26 3:14 ` [PATCH 2/3] efi/efi_test: fix the uninitialized value datasize Ivan Hu
[not found] ` <1474859691-8574-3-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-10-03 20:23 ` Matt Fleming
2016-09-26 3:14 ` [PATCH 3/3] efi/efi_test: fix the uninitialized value rv Ivan Hu
[not found] ` <1474859691-8574-4-git-send-email-ivan.hu-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2016-10-03 20:25 ` Matt Fleming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).