| Message ID | 1440488835-4388-1-git-send-email-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-08-25 at 15:47 +0800, Jason Wang wrote: > All fields of kvm_io_range were initialized or copied explicitly > afterwards. So switch to use kmalloc(). Is there any compiler added alignment padding in either structure? If so, those padding areas would now be uninitialized and may leak kernel data if copied to user-space. > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c [] > @@ -3248,7 +3248,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > if (bus->dev_count - bus->ioeventfd_count > NR_IOBUS_DEVS - 1) > return -ENOSPC; > > - new_bus = kzalloc(sizeof(*bus) + ((bus->dev_count + 1) * > + new_bus = kmalloc(sizeof(*bus) + ((bus->dev_count + 1) * > sizeof(struct kvm_io_range)), GFP_KERNEL); > if (!new_bus) > return -ENOMEM; > @@ -3280,7 +3280,7 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > if (r) > return r; > > - new_bus = kzalloc(sizeof(*bus) + ((bus->dev_count - 1) * > + new_bus = kmalloc(sizeof(*bus) + ((bus->dev_count - 1) * > sizeof(struct kvm_io_range)), GFP_KERNEL); > if (!new_bus) > return -ENOMEM; -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/25/2015 11:29 PM, Joe Perches wrote: > On Tue, 2015-08-25 at 15:47 +0800, Jason Wang wrote: >> > All fields of kvm_io_range were initialized or copied explicitly >> > afterwards. So switch to use kmalloc(). > Is there any compiler added alignment padding > in either structure? If so, those padding > areas would now be uninitialized and may leak > kernel data if copied to user-space. > I get your concern, but I don't a way to copy them to userspace, did you? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-08-26 at 13:39 +0800, Jason Wang wrote: > > On 08/25/2015 11:29 PM, Joe Perches wrote: > > On Tue, 2015-08-25 at 15:47 +0800, Jason Wang wrote: > >> > All fields of kvm_io_range were initialized or copied explicitly > >> > afterwards. So switch to use kmalloc(). > > Is there any compiler added alignment padding > > in either structure? If so, those padding > > areas would now be uninitialized and may leak > > kernel data if copied to user-space. > > > I get your concern, but I don't a way to copy them to userspace, did you? I didn't look. I just wanted you to be aware there's a difference and a reason why kzalloc might be used even though all structure members are initialized. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/26/2015 01:45 PM, Joe Perches wrote: > On Wed, 2015-08-26 at 13:39 +0800, Jason Wang wrote: >> > >> > On 08/25/2015 11:29 PM, Joe Perches wrote: >>> > > On Tue, 2015-08-25 at 15:47 +0800, Jason Wang wrote: >>>>> > >> > All fields of kvm_io_range were initialized or copied explicitly >>>>> > >> > afterwards. So switch to use kmalloc(). >>> > > Is there any compiler added alignment padding >>> > > in either structure? If so, those padding >>> > > areas would now be uninitialized and may leak >>> > > kernel data if copied to user-space. >>> > > >> > I get your concern, but I don't a way to copy them to userspace, did you? > I didn't look. > > I just wanted you to be aware there's a difference > and a reason why kzalloc might be used even though > all structure members are initialized. > I see, thanks for the reminding. Looks like we are safe and I will add something like "kvm_io_range was never accessed by userspace" in the commit log if there's a new version. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 8b8a444..0d79fe8 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3248,7 +3248,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, if (bus->dev_count - bus->ioeventfd_count > NR_IOBUS_DEVS - 1) return -ENOSPC; - new_bus = kzalloc(sizeof(*bus) + ((bus->dev_count + 1) * + new_bus = kmalloc(sizeof(*bus) + ((bus->dev_count + 1) * sizeof(struct kvm_io_range)), GFP_KERNEL); if (!new_bus) return -ENOMEM; @@ -3280,7 +3280,7 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, if (r) return r; - new_bus = kzalloc(sizeof(*bus) + ((bus->dev_count - 1) * + new_bus = kmalloc(sizeof(*bus) + ((bus->dev_count - 1) * sizeof(struct kvm_io_range)), GFP_KERNEL); if (!new_bus) return -ENOMEM;
All fields of kvm_io_range were initialized or copied explicitly afterwards. So switch to use kmalloc(). Cc: Gleb Natapov <gleb@kernel.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)