[v2,2/2] KVM: s390: Use memdup_user() rather than duplicating code

Commit Message

SF Markus Elfring Aug. 24, 2016, 6:40 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Wed, 24 Aug 2016 20:10:09 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Return directly if this copy operation failed.

Reviewed-by: David Hildenbrand <[e-mail]>
Signed-off-by: Markus Elfring <[e-mail]>
---

v2: Rebased on source files from "Linux next-20160824".

 arch/s390/kvm/guestdbg.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

Comments

Cornelia Huck Aug. 25, 2016, 4:11 p.m. UTC | #1

On Wed, 24 Aug 2016 20:40:03 +0200
SF Markus Elfring <[e-mail]> wrote:

> From: Markus Elfring <[e-mail]>
> Date: Wed, 24 Aug 2016 20:10:09 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Return directly if this copy operation failed.
> 
> Reviewed-by: David Hildenbrand <[e-mail]>
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
> 
> v2: Rebased on source files from "Linux next-20160824".
> 
>  arch/s390/kvm/guestdbg.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

Acked-by: Cornelia Huck <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Oct. 3, 2016, 12:11 p.m. UTC | #2

Hi Markus,

On Wed, Aug 24, 2016 at 8:40 PM, SF Markus Elfring
<[e-mail]> wrote:
> From: Markus Elfring <[e-mail]>
> Date: Wed, 24 Aug 2016 20:10:09 +0200
>
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
>
>   This issue was detected by using the Coccinelle software.
>
> * Return directly if this copy operation failed.
>
> Reviewed-by: David Hildenbrand <[e-mail]>
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>
> v2: Rebased on source files from "Linux next-20160824".
>
>  arch/s390/kvm/guestdbg.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 70b71ac..d7c6a7f 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -216,20 +216,10 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>         else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>                 return -EINVAL;
>
> -       bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
> -                               sizeof(*bp_data),
> -                               GFP_KERNEL);

Probably not an issue here, but if "sizeof(*bp_data) * dbg->arch.nr_hw_bp"
overflows, kmalloc_array() would have returned NULL here...

> -       if (!bp_data) {
> -               ret = -ENOMEM;
> -               goto error;
> -       }
> -
> -       if (copy_from_user(bp_data,
> -                          dbg->arch.hw_bp,
> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
> -               ret = -EFAULT;
> -               goto error;
> -       }
> +       bp_data = memdup_user(dbg->arch.hw_bp,
> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);

... while this would continue silently, and corrupt memory.

> +       if (IS_ERR(bp_data))
> +               return PTR_ERR(bp_data);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SF Markus Elfring Oct. 3, 2016, 12:28 p.m. UTC | #3

>> -       if (!bp_data) {
>> -               ret = -ENOMEM;
>> -               goto error;
>> -       }
>> -
>> -       if (copy_from_user(bp_data,
>> -                          dbg->arch.hw_bp,
>> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
>> -               ret = -EFAULT;
>> -               goto error;
>> -       }
>> +       bp_data = memdup_user(dbg->arch.hw_bp,
>> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);
> 
> ... while this would continue silently,

How do you think about to explain this information a bit more?


> and corrupt memory.

I wonder about this conclusion at the moment.

Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
in this update suggestion?

How does your feedback fit to the tag "Acked-by: Cornelia Huck"
from 2016-08-25?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Oct. 3, 2016, 1:10 p.m. UTC | #4

Hi Markus,

On Mon, Oct 3, 2016 at 2:28 PM, SF Markus Elfring
<[e-mail]> wrote:
>>> -       if (!bp_data) {
>>> -               ret = -ENOMEM;
>>> -               goto error;
>>> -       }
>>> -
>>> -       if (copy_from_user(bp_data,
>>> -                          dbg->arch.hw_bp,
>>> -                          sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
>>> -               ret = -EFAULT;
>>> -               goto error;
>>> -       }
>>> +       bp_data = memdup_user(dbg->arch.hw_bp,
>>> +                             sizeof(*bp_data) * dbg->arch.nr_hw_bp);
>>
>> ... while this would continue silently,
>
> How do you think about to explain this information a bit more?

kmalloc_array() has a builtin check for overflow while calculating the size.
This is the real reason why it's better to use kmalloc_array() than
kzalloc(n * size). If "n * size" overflow, kzalloc(n * size) would allocate a
memory block with a bogus size.

>> and corrupt memory.
>
> I wonder about this conclusion at the moment.
>
> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
> in this update suggestion?

Yes, but bp_data may still be a valid (as in "not an error") value.

Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
 a builtin overflow check, and will return NULL if overflow is detected.
However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
duplicating code") dropped that safety net again.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SF Markus Elfring Oct. 3, 2016, 1:47 p.m. UTC | #5

>> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
>> in this update suggestion?
> 
> Yes, but bp_data may still be a valid (as in "not an error") value.

Thanks for your constructive feedback.


> Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
> kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
>  a builtin overflow check, and will return NULL if overflow is detected.
> However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
> duplicating code") dropped that safety net again.

* How much are you concerned about the shown software evolution around
  multiplications for memory allocations?

* Does there really a probability remain that an inappropriate product
  would be calculated here (as the situation was before my two update steps
  for this software module)?

* Can it be that you are looking for a variant of a function like "memdup_user"
  where values can be passed as separate parameters "count" and "size" so that
  the needed multiplication and corresponding overflow check would be performed
  together as desired?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Oct. 3, 2016, 2 p.m. UTC | #6

Hi Markus,

On Mon, Oct 3, 2016 at 3:47 PM, SF Markus Elfring
<[e-mail]> wrote:
>>> Did you notice the check "IS_ERR(bp_data)" and the corresponding reaction
>>> in this update suggestion?
>>
>> Yes, but bp_data may still be a valid (as in "not an error") value.
>
> Thanks for your constructive feedback.
>
>> Your commit a1708a2eaded836b ("KVM: s390: Improve determination of sizes in
>> kvm_s390_import_bp_data()") made the code more robust, as kmalloc_array() ha
>>  a builtin overflow check, and will return NULL if overflow is detected.
>> However, commit 0624a8eb82efd58e ("KVM: s390: Use memdup_user() rather than
>> duplicating code") dropped that safety net again.
>
> * How much are you concerned about the shown software evolution around
>   multiplications for memory allocations?

Enough to write my reply ;-)

> * Does there really a probability remain that an inappropriate product
>   would be calculated here (as the situation was before my two update steps
>   for this software module)?

Perhaps not. Hence my "Probably not an issue here, ...".

> * Can it be that you are looking for a variant of a function like "memdup_user"
>   where values can be passed as separate parameters "count" and "size" so that
>   the needed multiplication and corresponding overflow check would be performed
>   together as desired?

If there are enough uses, and people like it, adding memdup_user_array()
may be a good idea...

P.S. Why do your questions make me think of a scientific paper? ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SF Markus Elfring Oct. 3, 2016, 2:25 p.m. UTC | #7

>> * Does there really a probability remain that an inappropriate product
>>   would be calculated here (as the situation was before my two update steps
>>   for this software module)?
> 
> Perhaps not. Hence my "Probably not an issue here, ...".

Thanks for your clarification.


>> * Can it be that you are looking for a variant of a function like "memdup_user"
>>   where values can be passed as separate parameters "count" and "size" so that
>>   the needed multiplication and corresponding overflow check would be performed
>>   together as desired?
> 
> If there are enough uses, and people like it, adding memdup_user_array()
> may be a good idea...

How are the chances of such an addition for the Linux programming interface?


> P.S. Why do your questions make me think of a scientific paper? ;-)

Would you like to recommend any for further reading?   ;-)

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 70b71ac..d7c6a7f 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -216,20 +216,10 @@  int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	bp_data = kmalloc_array(dbg->arch.nr_hw_bp,
-				sizeof(*bp_data),
-				GFP_KERNEL);
-	if (!bp_data) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	if (copy_from_user(bp_data,
-			   dbg->arch.hw_bp,
-			   sizeof(*bp_data) * dbg->arch.nr_hw_bp)) {
-		ret = -EFAULT;
-		goto error;
-	}
+	bp_data = memdup_user(dbg->arch.hw_bp,
+			      sizeof(*bp_data) * dbg->arch.nr_hw_bp);
+	if (IS_ERR(bp_data))
+		return PTR_ERR(bp_data);
 
 	for (i = 0; i < dbg->arch.nr_hw_bp; i++) {
 		switch (bp_data[i].type) {