Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1494853 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than  duplicating code Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-03 14:20 +0200
    Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than  duplicating code SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-03 14:30 +0200
      Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than  duplicating code Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-03 15:20 +0200
        Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than  duplicating code SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-03 15:50 +0200
          Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than  duplicating code Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-03 16:10 +0200
            Re: KVM: s390: Use memdup_user() rather than duplicating code SF Markus Elfring <elfring@users.sourceforge.net> - 2016-10-03 16:30 +0200

#1494853 — Re: [PATCH v2 2/2] KVM: s390: Use memdup_user() rather than duplicating code

Hi Markus,

On Wed, Aug 24, 2016 at 8:40 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <dahi@linux.vnet.ibm.com>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> 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 -- geert@linux-m68k.org

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

[toc] | [next] | [standalone]

#1494855

>> -       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

[toc] | [prev] | [next] | [standalone]

#1494878

Hi Markus,

On Mon, Oct 3, 2016 at 2:28 PM, SF Markus Elfring
<elfring@users.sourceforge.net> 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 -- geert@linux-m68k.org

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

[toc] | [prev] | [next] | [standalone]

#1494884

>> 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

[toc] | [prev] | [next] | [standalone]

#1494887

Hi Markus,

On Mon, Oct 3, 2016 at 3:47 PM, SF Markus Elfring
<elfring@users.sourceforge.net> 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 -- geert@linux-m68k.org

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

[toc] | [prev] | [next] | [standalone]

#1494898 — Re: KVM: s390: Use memdup_user() rather than duplicating code

>> * 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web