Hi Markus, On Mon, Oct 3, 2016 at 2:28 PM, SF Markus Elfring <elfr...@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 -- ge...@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