| Message ID | 1474275971-3546-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/19/16 11:06, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > virtio back end uses set of buffers to facilitate I/O operations. > If its size is too large, 'cpu_physical_memory_map' could return > a null address. This would result in a null dereference while > un-mapping descriptors. Add check to avoid it. > > Reported-by: Qinghao Tang <luodalongde@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/virtio/virtio.c | 5 +++++ > 1 file changed, 5 insertions(+) > > Update per: > -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03889.html > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 15ee3a7..311dd0b 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove > } > > iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); > + if (!iov[num_sg].iov_base) { > + error_report("virtio: bogus descriptor or out of resources"); > + exit(EXIT_FAILURE); > + } > + > iov[num_sg].iov_len = len; > addr[num_sg] = pa; > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
On Mon, Sep 19, 2016 at 02:36:11PM +0530, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > virtio back end uses set of buffers to facilitate I/O operations. > If its size is too large, 'cpu_physical_memory_map' could return > a null address. This would result in a null dereference while > un-mapping descriptors. Add check to avoid it. > > Reported-by: Qinghao Tang <luodalongde@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/virtio/virtio.c | 5 +++++ > 1 file changed, 5 insertions(+) > > Update per: > -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03889.html > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 15ee3a7..311dd0b 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove > } > > iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); > + if (!iov[num_sg].iov_base) { > + error_report("virtio: bogus descriptor or out of resources"); > + exit(EXIT_FAILURE); exit(1) is used everywhere else in the file (including just a few lines above in the same function). Please use exit(1) for consistency. Looks fine otherwise.
On 09/19/16 17:55, Stefan Hajnoczi wrote: > On Mon, Sep 19, 2016 at 02:36:11PM +0530, P J P wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> virtio back end uses set of buffers to facilitate I/O operations. >> If its size is too large, 'cpu_physical_memory_map' could return >> a null address. This would result in a null dereference while >> un-mapping descriptors. Add check to avoid it. >> >> Reported-by: Qinghao Tang <luodalongde@gmail.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/virtio/virtio.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> Update per: >> -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03889.html >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 15ee3a7..311dd0b 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove >> } >> >> iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); >> + if (!iov[num_sg].iov_base) { >> + error_report("virtio: bogus descriptor or out of resources"); >> + exit(EXIT_FAILURE); > > exit(1) is used everywhere else in the file (including just a few lines > above in the same function). Please use exit(1) for consistency. Laurent's pending series [Qemu-devel] [PATCH 00/26] trivial: use exit(EXIT_SUCCESS) and exit(EXIT_FAILURE) specifically his patch [Qemu-devel] [PATCH 11/26] pci, virtio: use exit(EXIT_SUCCESS) and exit(EXIT_FAILURE) converts the argument of the exit() that you name to EXIT_FAILURE. So using EXIT_FAILURE in Prasad's patch is actually what will uphold consistency. > Looks fine otherwise. > Thanks! Laszlo
On 19/09/2016 18:59, Laszlo Ersek wrote: > On 09/19/16 17:55, Stefan Hajnoczi wrote: >> On Mon, Sep 19, 2016 at 02:36:11PM +0530, P J P wrote: >>> From: Prasad J Pandit <pjp@fedoraproject.org> >>> >>> virtio back end uses set of buffers to facilitate I/O operations. >>> If its size is too large, 'cpu_physical_memory_map' could return >>> a null address. This would result in a null dereference while >>> un-mapping descriptors. Add check to avoid it. >>> >>> Reported-by: Qinghao Tang <luodalongde@gmail.com> >>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >>> --- >>> hw/virtio/virtio.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> Update per: >>> -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03889.html >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 15ee3a7..311dd0b 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove >>> } >>> >>> iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); >>> + if (!iov[num_sg].iov_base) { >>> + error_report("virtio: bogus descriptor or out of resources"); >>> + exit(EXIT_FAILURE); >> >> exit(1) is used everywhere else in the file (including just a few lines >> above in the same function). Please use exit(1) for consistency. > > Laurent's pending series > > [Qemu-devel] [PATCH 00/26] trivial: use exit(EXIT_SUCCESS) and > exit(EXIT_FAILURE) > > specifically his patch > > [Qemu-devel] [PATCH 11/26] pci, virtio: use exit(EXIT_SUCCESS) and > exit(EXIT_FAILURE) > > converts the argument of the exit() that you name to EXIT_FAILURE. > > So using EXIT_FAILURE in Prasad's patch is actually what will uphold > consistency. > >> Looks fine otherwise. >> Laszlo, it seems Peter (and some others) would prefer to use exit(1) instead of exit(EXIT_FAILURE), so I don't think my series will be applied. Laurent
On 09/19/16 19:16, Laurent Vivier wrote: > > > On 19/09/2016 18:59, Laszlo Ersek wrote: >> On 09/19/16 17:55, Stefan Hajnoczi wrote: >>> On Mon, Sep 19, 2016 at 02:36:11PM +0530, P J P wrote: >>>> From: Prasad J Pandit <pjp@fedoraproject.org> >>>> >>>> virtio back end uses set of buffers to facilitate I/O operations. >>>> If its size is too large, 'cpu_physical_memory_map' could return >>>> a null address. This would result in a null dereference while >>>> un-mapping descriptors. Add check to avoid it. >>>> >>>> Reported-by: Qinghao Tang <luodalongde@gmail.com> >>>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >>>> --- >>>> hw/virtio/virtio.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> Update per: >>>> -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03889.html >>>> >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>> index 15ee3a7..311dd0b 100644 >>>> --- a/hw/virtio/virtio.c >>>> +++ b/hw/virtio/virtio.c >>>> @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove >>>> } >>>> >>>> iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); >>>> + if (!iov[num_sg].iov_base) { >>>> + error_report("virtio: bogus descriptor or out of resources"); >>>> + exit(EXIT_FAILURE); >>> >>> exit(1) is used everywhere else in the file (including just a few lines >>> above in the same function). Please use exit(1) for consistency. >> >> Laurent's pending series >> >> [Qemu-devel] [PATCH 00/26] trivial: use exit(EXIT_SUCCESS) and >> exit(EXIT_FAILURE) >> >> specifically his patch >> >> [Qemu-devel] [PATCH 11/26] pci, virtio: use exit(EXIT_SUCCESS) and >> exit(EXIT_FAILURE) >> >> converts the argument of the exit() that you name to EXIT_FAILURE. >> >> So using EXIT_FAILURE in Prasad's patch is actually what will uphold >> consistency. >> >>> Looks fine otherwise. >>> > > Laszlo, it seems Peter (and some others) would prefer to use exit(1) > instead of exit(EXIT_FAILURE), so I don't think my series will be applied. Thanks for the information, Laurent! Prasad, can you please send a v3 then, to address Stefan's feedback? Thank you Laszlo
+-- On Mon, 19 Sep 2016, Laszlo Ersek wrote --+ | Prasad, can you please send a v3 then, to address Stefan's feedback? Yes, sent patch v3. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 15ee3a7..311dd0b 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -472,6 +472,11 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove } iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write); + if (!iov[num_sg].iov_base) { + error_report("virtio: bogus descriptor or out of resources"); + exit(EXIT_FAILURE); + } + iov[num_sg].iov_len = len; addr[num_sg] = pa;