From: Longpeng <longpeng2@huawei.com>
vhost_log_alloc() may fails and returned pointer of log is null.
However there're two places derefernce the return pointer without
check.
Signed-off-by: Longpeng <longpeng2@huawei.com>
---
hw/virtio/vhost.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9edfadc..c7ad6e5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
if (!log || log->size != size) {
log = vhost_log_alloc(size, share);
+ if (!log) {
+ return NULL;
+ }
+
if (share) {
vhost_log_shm = log;
} else {
@@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
{
- struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
- uint64_t log_base = (uintptr_t)log->log;
+ struct vhost_log *log;
+ uint64_t log_base;
int r;
+ log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
+ if (!log) {
+ return;
+ }
+
+ log_base = (uintptr_t)log->log;
+
/* inform backend of log switching, this must be done before
releasing the current log, to ensure no logging is lost */
r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
@@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
hdev->log_size = vhost_get_log_size(hdev);
hdev->log = vhost_log_get(hdev->log_size,
vhost_dev_log_is_shared(hdev));
+ if (!hdev->log) {
+ goto fail_vq;
+ }
+
log_base = (uintptr_t)hdev->log->log;
r = hdev->vhost_ops->vhost_set_log_base(hdev,
hdev->log_size ? log_base : 0,
--
1.8.3.1
Hi Michael,
ping...
On 2020/2/24 14:42, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
>
> vhost_log_alloc() may fails and returned pointer of log is null.
> However there're two places derefernce the return pointer without
> check.
>
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> hw/virtio/vhost.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9edfadc..c7ad6e5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>
> if (!log || log->size != size) {
> log = vhost_log_alloc(size, share);
> + if (!log) {
> + return NULL;
> + }
> +
> if (share) {
> vhost_log_shm = log;
> } else {
> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>
> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> {
> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> - uint64_t log_base = (uintptr_t)log->log;
> + struct vhost_log *log;
> + uint64_t log_base;
> int r;
>
> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> + if (!log) {
> + return;
> + }
> +
> + log_base = (uintptr_t)log->log;
> +
> /* inform backend of log switching, this must be done before
> releasing the current log, to ensure no logging is lost */
> r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> @@ -1640,6 +1651,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> hdev->log_size = vhost_get_log_size(hdev);
> hdev->log = vhost_log_get(hdev->log_size,
> vhost_dev_log_is_shared(hdev));
> + if (!hdev->log) {
> + goto fail_vq;
> + }
> +
> log_base = (uintptr_t)hdev->log->log;
> r = hdev->vhost_ops->vhost_set_log_base(hdev,
> hdev->log_size ? log_base : 0,
>
--
---
Regards,
Longpeng(Mike)
On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
>
> vhost_log_alloc() may fails and returned pointer of log is null.
> However there're two places derefernce the return pointer without
> check.
>
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> hw/virtio/vhost.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9edfadc..c7ad6e5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>
> if (!log || log->size != size) {
> log = vhost_log_alloc(size, share);
> + if (!log) {
> + return NULL;
> + }
> +
> if (share) {
> vhost_log_shm = log;
> } else {
> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>
> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> {
> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> - uint64_t log_base = (uintptr_t)log->log;
> + struct vhost_log *log;
> + uint64_t log_base;
> int r;
>
> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> + if (!log) {
> + return;
> + }
> +
I'm not sure silently failing like this is safe. Callers assume
log can be resized. What can be done? I suspect not much
beside exiting ...
Speaking of which, lots of other failures in log resizing
path seem to be silently ignored.
I guess we should propagate them, and fix callers to check
the return code?
On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
>> From: Longpeng <longpeng2@huawei.com>
>>
>> vhost_log_alloc() may fails and returned pointer of log is null.
>> However there're two places derefernce the return pointer without
>> check.
>>
>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> ---
>> hw/virtio/vhost.c | 19 +++++++++++++++++--
>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 9edfadc..c7ad6e5 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>>
>> if (!log || log->size != size) {
>> log = vhost_log_alloc(size, share);
>> + if (!log) {
>> + return NULL;
>> + }
>> +
>> if (share) {
>> vhost_log_shm = log;
>> } else {
>> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>>
>> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>> {
>> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>> - uint64_t log_base = (uintptr_t)log->log;
>> + struct vhost_log *log;
>> + uint64_t log_base;
>> int r;
>>
>> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>> + if (!log) {
>> + return;
>> + }
>> +
>
> I'm not sure silently failing like this is safe. Callers assume
> log can be resized. What can be done? I suspect not much
> beside exiting ...
> Speaking of which, lots of other failures in log resizing
> path seem to be silently ignored.
> I guess we should propagate them, and fix callers to check
> the return code?
>
How about to let the callers treat the failure of log_resize as a fatal error ?
-static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
+static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
{
- struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
- uint64_t log_base = (uintptr_t)log->log;
+ struct vhost_log *log;
+ uint64_t log_base;
int r;
+ log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
+ if (!log) {
+ r = -1;
+ goto out;
+ }
+
+ log_base = (uintptr_t)log->log;
+
/* inform backend of log switching, this must be done before
releasing the current log, to ensure no logging is lost */
r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
@@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev
*dev, uint64_t size)
vhost_log_put(dev, true);
dev->log = log;
dev->log_size = size;
+
+out:
+ return 0;
}
@@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
#define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
/* To log more, must increase log size before table update. */
if (dev->log_size < log_size) {
- vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
+ if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
+ abort();
+ }
}
r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
if (r < 0) {
@@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
}
/* To log less, can only decrease log size after table update. */
if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
- vhost_dev_log_resize(dev, log_size);
+ if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
+ abort();
+ }
}
out:
@@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
int enable)
}
vhost_log_put(dev, false);
} else {
- vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+ r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
+ if (r < 0) {
+ return r;
+ }
+
r = vhost_dev_set_log(dev, true);
if (r < 0) {
return r;
>
> .
>
--
---
Regards,
Longpeng(Mike)
On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> > On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> >> From: Longpeng <longpeng2@huawei.com>
> >>
> >> vhost_log_alloc() may fails and returned pointer of log is null.
> >> However there're two places derefernce the return pointer without
> >> check.
> >>
> >> Signed-off-by: Longpeng <longpeng2@huawei.com>
> >> ---
> >> hw/virtio/vhost.c | 19 +++++++++++++++++--
> >> 1 file changed, 17 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 9edfadc..c7ad6e5 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -219,6 +219,10 @@ static struct vhost_log *vhost_log_get(uint64_t size, bool share)
> >>
> >> if (!log || log->size != size) {
> >> log = vhost_log_alloc(size, share);
> >> + if (!log) {
> >> + return NULL;
> >> + }
> >> +
> >> if (share) {
> >> vhost_log_shm = log;
> >> } else {
> >> @@ -270,10 +274,17 @@ static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> >>
> >> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> >> {
> >> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >> - uint64_t log_base = (uintptr_t)log->log;
> >> + struct vhost_log *log;
> >> + uint64_t log_base;
> >> int r;
> >>
> >> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >> + if (!log) {
> >> + return;
> >> + }
> >> +
> >
> > I'm not sure silently failing like this is safe. Callers assume
> > log can be resized. What can be done? I suspect not much
> > beside exiting ...
> > Speaking of which, lots of other failures in log resizing
> > path seem to be silently ignored.
> > I guess we should propagate them, and fix callers to check
> > the return code?
> >
> How about to let the callers treat the failure of log_resize as a fatal error ?
>
> -static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> +static inline int vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> {
> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> - uint64_t log_base = (uintptr_t)log->log;
> + struct vhost_log *log;
> + uint64_t log_base;
> int r;
>
> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> + if (!log) {
> + r = -1;
> + goto out;
> + }
> +
> + log_base = (uintptr_t)log->log;
> +
> /* inform backend of log switching, this must be done before
> releasing the current log, to ensure no logging is lost */
> r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> @@ -284,6 +296,9 @@ static inline void vhost_dev_log_resize(struct vhost_dev
> *dev, uint64_t size)
> vhost_log_put(dev, true);
> dev->log = log;
> dev->log_size = size;
> +
> +out:
> + return 0;
> }
>
>
> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
> #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
> /* To log more, must increase log size before table update. */
> if (dev->log_size < log_size) {
> - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> + abort();
> + }
> }
> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> if (r < 0) {
> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
> }
> /* To log less, can only decrease log size after table update. */
> if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> - vhost_dev_log_resize(dev, log_size);
> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> + abort();
> + }
> }
>
> out:
I think the suggested handling is
error_report() and exit().
we also need to propagate errno. So how about passing in Error then?
> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
> int enable)
> }
> vhost_log_put(dev, false);
> } else {
> - vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> + if (r < 0) {
> + return r;
> + }
> +
> r = vhost_dev_set_log(dev, true);
> if (r < 0) {
> return r;
>
>
> >
> > .
> >
>
> --
> ---
> Regards,
> Longpeng(Mike)
在 2020/3/10 16:23, Michael S. Tsirkin 写道:
> On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>>
>>
>> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
>>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
>>>> From: Longpeng <longpeng2@huawei.com>
>>>>
>>>> vhost_log_alloc() may fails and returned pointer of log is null.
>>>> However there're two places derefernce the return pointer without
>>>> check.
>>>>
[...]
>>>> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>>>> {
>>>> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>>>> - uint64_t log_base = (uintptr_t)log->log;
>>>> + struct vhost_log *log;
>>>> + uint64_t log_base;
>>>> int r;
>>>>
>>>> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>>>> + if (!log) {
>>>> + return;
>>>> + }
>>>> +
>>>
>>> I'm not sure silently failing like this is safe. Callers assume
>>> log can be resized. What can be done? I suspect not much
>>> beside exiting ...
>>> Speaking of which, lots of other failures in log resizing
>>> path seem to be silently ignored.
>>> I guess we should propagate them, and fix callers to check
>>> the return code?
>>>
>> How about to let the callers treat the failure of log_resize as a fatal error ?
>>
[...]
>>
>> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
>> #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
>> /* To log more, must increase log size before table update. */
>> if (dev->log_size < log_size) {
>> - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
>> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> + abort();
>> + }
>> }
>> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
>> if (r < 0) {
>> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
>> }
>> /* To log less, can only decrease log size after table update. */
>> if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
>> - vhost_dev_log_resize(dev, log_size);
>> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
>> + abort();
>> + }
>> }
>>
>> out:
>
>
> I think the suggested handling is
> error_report() and exit().
> we also need to propagate errno. So how about passing in Error then?
>
vhost_dev_log_resize
vhost_log_get
vhost_log_alloc
error_report_err (fail path, errno is in the errp)
VHOST_OPS_DEBUG (if ->vhost_set_log_base fail)
error_report (errno)
Um, it seems log_resize will report error with errno internal, do we need
error_report once more ?
>
>> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
>> int enable)
>> }
>> vhost_log_put(dev, false);
>> } else {
>> - vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
>> + if (r < 0) {
>> + return r;
>> + }
>> +
>> r = vhost_dev_set_log(dev, true);
>> if (r < 0) {
>> return r;
>>
>>
>>>
>>> .
>>>
>>
>> --
>> ---
>> Regards,
>> Longpeng(Mike)
>
>
> .
>
--
Regards,
Longpeng(Mike)
On Tue, Mar 10, 2020 at 08:02:30PM +0800, Longpeng (Mike) wrote:
> 在 2020/3/10 16:23, Michael S. Tsirkin 写道:
> > On Tue, Mar 10, 2020 at 04:04:35PM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
> >>
> >>
> >> On 2020/3/10 13:57, Michael S. Tsirkin wrote:
> >>> On Mon, Feb 24, 2020 at 02:42:18PM +0800, Longpeng(Mike) wrote:
> >>>> From: Longpeng <longpeng2@huawei.com>
> >>>>
> >>>> vhost_log_alloc() may fails and returned pointer of log is null.
> >>>> However there're two places derefernce the return pointer without
> >>>> check.
> >>>>
> [...]
>
> >>>> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> >>>> {
> >>>> - struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >>>> - uint64_t log_base = (uintptr_t)log->log;
> >>>> + struct vhost_log *log;
> >>>> + uint64_t log_base;
> >>>> int r;
> >>>>
> >>>> + log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
> >>>> + if (!log) {
> >>>> + return;
> >>>> + }
> >>>> +
> >>>
> >>> I'm not sure silently failing like this is safe. Callers assume
> >>> log can be resized. What can be done? I suspect not much
> >>> beside exiting ...
> >>> Speaking of which, lots of other failures in log resizing
> >>> path seem to be silently ignored.
> >>> I guess we should propagate them, and fix callers to check
> >>> the return code?
> >>>
> >> How about to let the callers treat the failure of log_resize as a fatal error ?
> >>
> [...]
>
> >>
> >> @@ -510,7 +525,9 @@ static void vhost_commit(MemoryListener *listener)
> >> #define VHOST_LOG_BUFFER (0x1000 / sizeof *dev->log)
> >> /* To log more, must increase log size before table update. */
> >> if (dev->log_size < log_size) {
> >> - vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> >> + abort();
> >> + }
> >> }
> >> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> >> if (r < 0) {
> >> @@ -518,7 +535,9 @@ static void vhost_commit(MemoryListener *listener)
> >> }
> >> /* To log less, can only decrease log size after table update. */
> >> if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> >> - vhost_dev_log_resize(dev, log_size);
> >> + if (vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER) < 0) {
> >> + abort();
> >> + }
> >> }
> >>
> >> out:
> >
> >
> > I think the suggested handling is
> > error_report() and exit().
> > we also need to propagate errno. So how about passing in Error then?
> >
> vhost_dev_log_resize
> vhost_log_get
> vhost_log_alloc
> error_report_err (fail path, errno is in the errp)
> VHOST_OPS_DEBUG (if ->vhost_set_log_base fail)
> error_report (errno)
>
> Um, it seems log_resize will report error with errno internal, do we need
> error_report once more ?
Well we need to convert this over to something other than
VHOST_OPS_DEBUG, that will go away at some point.
> >
> >> @@ -818,7 +837,11 @@ static int vhost_migration_log(MemoryListener *listener,
> >> int enable)
> >> }
> >> vhost_log_put(dev, false);
> >> } else {
> >> - vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> >> + r = vhost_dev_log_resize(dev, vhost_get_log_size(dev));
> >> + if (r < 0) {
> >> + return r;
> >> + }
> >> +
> >> r = vhost_dev_set_log(dev, true);
> >> if (r < 0) {
> >> return r;
> >>
> >>
> >>>
> >>> .
> >>>
> >>
> >> --
> >> ---
> >> Regards,
> >> Longpeng(Mike)
> >
> >
> > .
> >
>
>
> --
> Regards,
> Longpeng(Mike)
© 2016 - 2025 Red Hat, Inc.