* [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address @ 2016-09-19 9:06 P J P 2016-09-19 10:37 ` Laszlo Ersek 2016-09-19 15:55 ` Stefan Hajnoczi 0 siblings, 2 replies; 7+ messages in thread From: P J P @ 2016-09-19 9:06 UTC (permalink / raw) To: Qemu Developers Cc: Qinghao Tang, Laszlo Ersek, Michael S. Tsirkin, Stefan Hajnoczi, Prasad J Pandit 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; -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 9:06 [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address P J P @ 2016-09-19 10:37 ` Laszlo Ersek 2016-09-19 15:55 ` Stefan Hajnoczi 1 sibling, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2016-09-19 10:37 UTC (permalink / raw) To: P J P, Qemu Developers Cc: Qinghao Tang, Michael S. Tsirkin, Stefan Hajnoczi, Prasad J Pandit 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 9:06 [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address P J P 2016-09-19 10:37 ` Laszlo Ersek @ 2016-09-19 15:55 ` Stefan Hajnoczi 2016-09-19 16:59 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Stefan Hajnoczi @ 2016-09-19 15:55 UTC (permalink / raw) To: P J P Cc: Qemu Developers, Qinghao Tang, Laszlo Ersek, Michael S. Tsirkin, Prasad J Pandit [-- Attachment #1: Type: text/plain, Size: 1322 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 15:55 ` Stefan Hajnoczi @ 2016-09-19 16:59 ` Laszlo Ersek 2016-09-19 17:16 ` Laurent Vivier 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2016-09-19 16:59 UTC (permalink / raw) To: Stefan Hajnoczi, P J P Cc: Qemu Developers, Qinghao Tang, Michael S. Tsirkin, Prasad J Pandit, Laurent Vivier 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 16:59 ` Laszlo Ersek @ 2016-09-19 17:16 ` Laurent Vivier 2016-09-19 17:25 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Laurent Vivier @ 2016-09-19 17:16 UTC (permalink / raw) To: Laszlo Ersek, Stefan Hajnoczi, P J P Cc: Qemu Developers, Qinghao Tang, Michael S. Tsirkin, Prasad J Pandit, Peter Maydell 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 17:16 ` Laurent Vivier @ 2016-09-19 17:25 ` Laszlo Ersek 2016-09-19 18:26 ` P J P 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2016-09-19 17:25 UTC (permalink / raw) To: Laurent Vivier, Stefan Hajnoczi, P J P Cc: Qemu Developers, Qinghao Tang, Michael S. Tsirkin, Prasad J Pandit, Peter Maydell 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address 2016-09-19 17:25 ` Laszlo Ersek @ 2016-09-19 18:26 ` P J P 0 siblings, 0 replies; 7+ messages in thread From: P J P @ 2016-09-19 18:26 UTC (permalink / raw) To: Laszlo Ersek Cc: Laurent Vivier, Stefan Hajnoczi, Qemu Developers, Qinghao Tang, Michael S. Tsirkin, Peter Maydell +-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-19 18:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-19 9:06 [Qemu-devel] [PATCH v2] virtio: add check for descriptor's mapped address P J P 2016-09-19 10:37 ` Laszlo Ersek 2016-09-19 15:55 ` Stefan Hajnoczi 2016-09-19 16:59 ` Laszlo Ersek 2016-09-19 17:16 ` Laurent Vivier 2016-09-19 17:25 ` Laszlo Ersek 2016-09-19 18:26 ` P J P
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).