| Message ID | 20210224055401.492407-2-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Detect reentrant RX casue by loopback | expand |
On 2/24/21 6:53 AM, Jason Wang wrote: > Some NIC supports loopback mode and this is done by calling > nc->info->receive() directly which in fact suppresses the effort of > reentrancy check that is done in qemu_net_queue_send(). > > Unfortunately we can use qemu_net_queue_send() here since for loop > back there's no sender as peer, so this patch introduce a > qemu_receive_packet() which is used for implementing loopback mode > for a NIC with this check. IIUC the guest could trigger an infinite loop and brick the emulated device model. Likely exhausting the stack, so either SEGV by corruption or some ENOMEM? Since this is guest triggerable, shouldn't we contact qemu-security@ list and ask for a CVE for this issue, so distributions can track the patches to backport in their stable releases? (it seems to be within the KVM devices boundary). > > NIC that supports loopback mode will be converted to this helper. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > include/net/net.h | 5 +++++ > include/net/queue.h | 8 ++++++++ > net/net.c | 38 +++++++++++++++++++++++++++++++------- > net/queue.c | 22 ++++++++++++++++++++++ > 4 files changed, 66 insertions(+), 7 deletions(-)
On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > On 2/24/21 6:53 AM, Jason Wang wrote: >> Some NIC supports loopback mode and this is done by calling >> nc->info->receive() directly which in fact suppresses the effort of >> reentrancy check that is done in qemu_net_queue_send(). >> >> Unfortunately we can use qemu_net_queue_send() here since for loop >> back there's no sender as peer, so this patch introduce a >> qemu_receive_packet() which is used for implementing loopback mode >> for a NIC with this check. > IIUC the guest could trigger an infinite loop and brick the emulated > device model. Likely exhausting the stack, so either SEGV by > corruption or some ENOMEM? Yes. > > Since this is guest triggerable, shouldn't we contact qemu-security@ > list and ask for a CVE for this issue, so distributions can track > the patches to backport in their stable releases? (it seems to be > within the KVM devices boundary). That's the plan. I discussed this with Prasad before and he promise to ask CVE for this. But it's a knwon issue, the reentrant DMA which has been discussed before[1], unfortuantely we don't make any progress. This patch can only fix the NIC RX issue. Thanks [1] https://mail.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html > >> NIC that supports loopback mode will be converted to this helper. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> include/net/net.h | 5 +++++ >> include/net/queue.h | 8 ++++++++ >> net/net.c | 38 +++++++++++++++++++++++++++++++------- >> net/queue.c | 22 ++++++++++++++++++++++ >> 4 files changed, 66 insertions(+), 7 deletions(-)
On 2/24/21 2:17 PM, Jason Wang wrote: > > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: >> On 2/24/21 6:53 AM, Jason Wang wrote: >>> Some NIC supports loopback mode and this is done by calling >>> nc->info->receive() directly which in fact suppresses the effort of >>> reentrancy check that is done in qemu_net_queue_send(). >>> >>> Unfortunately we can use qemu_net_queue_send() here since for loop >>> back there's no sender as peer, so this patch introduce a >>> qemu_receive_packet() which is used for implementing loopback mode >>> for a NIC with this check. >> IIUC the guest could trigger an infinite loop and brick the emulated >> device model. Likely exhausting the stack, so either SEGV by >> corruption or some ENOMEM? > > > Yes. > > >> >> Since this is guest triggerable, shouldn't we contact qemu-security@ >> list and ask for a CVE for this issue, so distributions can track >> the patches to backport in their stable releases? (it seems to be >> within the KVM devices boundary). > > > That's the plan. I discussed this with Prasad before and he promise to > ask CVE for this. Good! We just need to be sure to amend the CVE number to the patches before committing them. > > But it's a knwon issue, the reentrant DMA which has been discussed > before[1], unfortuantely we don't make any progress. This patch can only > fix the NIC RX issue. > > Thanks > > [1] https://mail.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html
+-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ | On 2/24/21 2:17 PM, Jason Wang wrote: | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: | >> IIUC the guest could trigger an infinite loop and brick the emulated | >> device model. Likely exhausting the stack, so either SEGV by corruption | >> or some ENOMEM? | > | > Yes. | >> | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list | >> and ask for a CVE for this issue, so distributions can track the patches | >> to backport in their stable releases? (it seems to be within the KVM | >> devices boundary). | > | > | > That's the plan. I discussed this with Prasad before and he promise to | > ask CVE for this. 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 2/24/21 6:53 AM, Jason Wang wrote: > Some NIC supports loopback mode and this is done by calling > nc->info->receive() directly which in fact suppresses the effort of > reentrancy check that is done in qemu_net_queue_send(). > > Unfortunately we can use qemu_net_queue_send() here since for loop > back there's no sender as peer, so this patch introduce a > qemu_receive_packet() which is used for implementing loopback mode > for a NIC with this check. > > NIC that supports loopback mode will be converted to this helper. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > include/net/net.h | 5 +++++ > include/net/queue.h | 8 ++++++++ > net/net.c | 38 +++++++++++++++++++++++++++++++------- > net/queue.c | 22 ++++++++++++++++++++++ > 4 files changed, 66 insertions(+), 7 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 210225 1931, P J P wrote: > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ > | On 2/24/21 2:17 PM, Jason Wang wrote: > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > | >> IIUC the guest could trigger an infinite loop and brick the emulated > | >> device model. Likely exhausting the stack, so either SEGV by corruption > | >> or some ENOMEM? > | > > | > Yes. > | >> > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list > | >> and ask for a CVE for this issue, so distributions can track the patches > | >> to backport in their stable releases? (it seems to be within the KVM > | >> devices boundary). > | > > | > > | > That's the plan. I discussed this with Prasad before and he promise to > | > ask CVE for this. > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. > Hi Prasad, What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't those just manifestations of this bug for the e1000 and the eepro100 bug? -Alex > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 210225 1128, Alexander Bulekov wrote: > On 210225 1931, P J P wrote: > > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ > > | On 2/24/21 2:17 PM, Jason Wang wrote: > > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > > | >> IIUC the guest could trigger an infinite loop and brick the emulated > > | >> device model. Likely exhausting the stack, so either SEGV by corruption > > | >> or some ENOMEM? > > | > > > | > Yes. > > | >> > > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list > > | >> and ask for a CVE for this issue, so distributions can track the patches > > | >> to backport in their stable releases? (it seems to be within the KVM > > | >> devices boundary). > > | > > > | > > > | > That's the plan. I discussed this with Prasad before and he promise to > > | > ask CVE for this. > > > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. > > > > Hi Prasad, > What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't > those just manifestations of this bug for the e1000 and the eepro100 > bug? ^^ *devices links: https://www.openwall.com/lists/oss-security/2021/02/25/1 https://www.openwall.com/lists/oss-security/2021/02/25/2 > -Alex > > > Thank you. > > -- > > Prasad J Pandit / Red Hat Product Security Team > > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D >
Hello Alex, On Thursday, 25 February, 2021, 10:00:33 pm IST, Alexander Bulekov <alxndr@bu.edu> wrote: On 210225 1128, Alexander Bulekov wrote: > On 210225 1931, P J P wrote: > > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ > > | On 2/24/21 2:17 PM, Jason Wang wrote: > > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > > | >> IIUC the guest could trigger an infinite loop and brick the emulated > > | >> device model. Likely exhausting the stack, so either SEGV by corruption > > | >> or some ENOMEM? > > | > > > | > Yes. > > | >> > > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list > > | >> and ask for a CVE for this issue, so distributions can track the patches > > | >> to backport in their stable releases? (it seems to be within the KVM > > | >> devices boundary). > > | > > > | > > > | > That's the plan. I discussed this with Prasad before and he promise to > > | > ask CVE for this. > > > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. > > What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't > those just manifestations of this bug for the e1000 and the eepro100 > devices * You mean manifestations of the dam re-entrancy issue? * They have separate CVEs because they are fixed individually. Thank you. --- -P J P http://feedmug.com
On 210226 1814, P J P wrote: > Hello Alex, > > On Thursday, 25 February, 2021, 10:00:33 pm IST, Alexander Bulekov <alxndr@bu.edu> wrote: > On 210225 1128, Alexander Bulekov wrote: > > On 210225 1931, P J P wrote: > > > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+ > > > | On 2/24/21 2:17 PM, Jason Wang wrote: > > > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote: > > > | >> IIUC the guest could trigger an infinite loop and brick the emulated > > > | >> device model. Likely exhausting the stack, so either SEGV by corruption > > > | >> or some ENOMEM? > > > | > > > > | > Yes. > > > | >> > > > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list > > > | >> and ask for a CVE for this issue, so distributions can track the patches > > > | >> to backport in their stable releases? (it seems to be within the KVM > > > | >> devices boundary). > > > | > > > > | > > > > | > That's the plan. I discussed this with Prasad before and he promise to > > > | > ask CVE for this. > > > > > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc. > > > > What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't > > those just manifestations of this bug for the e1000 and the eepro100 > > devices > > * You mean manifestations of the dam re-entrancy issue? > Ah I got confused - those other CVEs don't seem to be related to loopback. -Alex > * They have separate CVEs because they are fixed individually. > > > Thank you. > --- > -P J P > http://feedmug.com
diff --git a/include/net/net.h b/include/net/net.h index 919facaad2..65eb8a58c5 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -144,12 +144,17 @@ void *qemu_get_nic_opaque(NetClientState *nc); void qemu_del_net_client(NetClientState *nc); typedef void (*qemu_nic_foreach)(NICState *nic, void *opaque); void qemu_foreach_nic(qemu_nic_foreach func, void *opaque); +int qemu_can_receive_packet(NetClientState *nc); int qemu_can_send_packet(NetClientState *nc); ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec *iov, int iovcnt); ssize_t qemu_sendv_packet_async(NetClientState *nc, const struct iovec *iov, int iovcnt, NetPacketSent *sent_cb); ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size); +ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf,int size); +ssize_t qemu_receive_packet_iov(NetClientState *nc, + const struct iovec *iov, + int iovcnt); ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size); ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf, int size, NetPacketSent *sent_cb); diff --git a/include/net/queue.h b/include/net/queue.h index c0269bb1dc..9f2f289d77 100644 --- a/include/net/queue.h +++ b/include/net/queue.h @@ -55,6 +55,14 @@ void qemu_net_queue_append_iov(NetQueue *queue, void qemu_del_net_queue(NetQueue *queue); +ssize_t qemu_net_queue_receive(NetQueue *queue, + const uint8_t *data, + size_t size); + +ssize_t qemu_net_queue_receive_iov(NetQueue *queue, + const struct iovec *iov, + int iovcnt); + ssize_t qemu_net_queue_send(NetQueue *queue, NetClientState *sender, unsigned flags, diff --git a/net/net.c b/net/net.c index e1035f21d1..6e470133ad 100644 --- a/net/net.c +++ b/net/net.c @@ -528,6 +528,17 @@ int qemu_set_vnet_be(NetClientState *nc, bool is_be) #endif } +int qemu_can_receive_packet(NetClientState *nc) +{ + if (nc->receive_disabled) { + return 0; + } else if (nc->info->can_receive && + !nc->info->can_receive(nc)) { + return 0; + } + return 1; +} + int qemu_can_send_packet(NetClientState *sender) { int vm_running = runstate_is_running(); @@ -540,13 +551,7 @@ int qemu_can_send_packet(NetClientState *sender) return 1; } - if (sender->peer->receive_disabled) { - return 0; - } else if (sender->peer->info->can_receive && - !sender->peer->info->can_receive(sender->peer)) { - return 0; - } - return 1; + return qemu_can_receive_packet(sender->peer); } static ssize_t filter_receive_iov(NetClientState *nc, @@ -679,6 +684,25 @@ ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size) return qemu_send_packet_async(nc, buf, size, NULL); } +ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf, int size) +{ + if (!qemu_can_receive_packet(nc)) { + return 0; + } + + return qemu_net_queue_receive(nc->incoming_queue, buf, size); +} + +ssize_t qemu_receive_packet_iov(NetClientState *nc, const struct iovec *iov, + int iovcnt) +{ + if (!qemu_can_receive_packet(nc)) { + return 0; + } + + return qemu_net_queue_receive_iov(nc->incoming_queue, iov, iovcnt); +} + ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size) { return qemu_send_packet_async_with_flags(nc, QEMU_NET_PACKET_FLAG_RAW, diff --git a/net/queue.c b/net/queue.c index 19e32c80fd..c872d51df8 100644 --- a/net/queue.c +++ b/net/queue.c @@ -182,6 +182,28 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue, return ret; } +ssize_t qemu_net_queue_receive(NetQueue *queue, + const uint8_t *data, + size_t size) +{ + if (queue->delivering) { + return 0; + } + + return qemu_net_queue_deliver(queue, NULL, 0, data, size); +} + +ssize_t qemu_net_queue_receive_iov(NetQueue *queue, + const struct iovec *iov, + int iovcnt) +{ + if (queue->delivering) { + return 0; + } + + return qemu_net_queue_deliver_iov(queue, NULL, 0, iov, iovcnt); +} + ssize_t qemu_net_queue_send(NetQueue *queue, NetClientState *sender, unsigned flags,
Some NIC supports loopback mode and this is done by calling nc->info->receive() directly which in fact suppresses the effort of reentrancy check that is done in qemu_net_queue_send(). Unfortunately we can use qemu_net_queue_send() here since for loop back there's no sender as peer, so this patch introduce a qemu_receive_packet() which is used for implementing loopback mode for a NIC with this check. NIC that supports loopback mode will be converted to this helper. Signed-off-by: Jason Wang <jasowang@redhat.com> --- include/net/net.h | 5 +++++ include/net/queue.h | 8 ++++++++ net/net.c | 38 +++++++++++++++++++++++++++++++------- net/queue.c | 22 ++++++++++++++++++++++ 4 files changed, 66 insertions(+), 7 deletions(-)