| Message ID | 20200303104724.233375-3-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: tulip: add checks to avoid OOB access | expand |
On Tue, Mar 03, 2020 at 04:17:24PM +0530, P J P wrote: > diff --git a/hw/net/tulip.c b/hw/net/tulip.c > index fbe40095da..757f12c710 100644 > --- a/hw/net/tulip.c > +++ b/hw/net/tulip.c > @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState *s, const uint8_t *addr) > return ret; > } > > +static int > +tulip_can_receive(NetClientState *nc) > +{ > + TULIPState *s = qemu_get_nic_opaque(nc); > + > + if (s->rx_frame_len || tulip_rx_stopped(s)) { > + return false; > + } > + > + return true; > +} Are the required qemu_flush_queued_packets() calls in place so that packet transfer wakes up again when .can_receive() transitions from false to true? (If qemu_flush_queued_packets() is missing then transmission hangs after .can_receive() becomes false.) Stefan
Hello Stefan, Jason,
+-- On Fri, 6 Mar 2020, Stefan Hajnoczi wrote --+
| > +static int
| > +tulip_can_receive(NetClientState *nc)
| > +{
| > + TULIPState *s = qemu_get_nic_opaque(nc);
| > +
| > + if (s->rx_frame_len || tulip_rx_stopped(s)) {
| > + return false;
| > + }
| > +
| > + return true;
| > +}
|
| Are the required qemu_flush_queued_packets() calls in place so that
| packet transfer wakes up again when .can_receive() transitions from
| false to true?
Yes, qemu_flush_queued_packets() calls are in tulip_write(). Do we need to
call tulip_can_receive() before each call?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 2020/3/17 上午2:01, P J P wrote: > Hello Stefan, Jason, > > +-- On Fri, 6 Mar 2020, Stefan Hajnoczi wrote --+ > | > +static int > | > +tulip_can_receive(NetClientState *nc) > | > +{ > | > + TULIPState *s = qemu_get_nic_opaque(nc); > | > + > | > + if (s->rx_frame_len || tulip_rx_stopped(s)) { > | > + return false; > | > + } > | > + > | > + return true; > | > +} > | > | Are the required qemu_flush_queued_packets() calls in place so that > | packet transfer wakes up again when .can_receive() transitions from > | false to true? > > Yes, qemu_flush_queued_packets() calls are in tulip_write(). Do we need to > call tulip_can_receive() before each call? Probably not, just need to make sure the check in tulip_rx_stopped(s) matches the action that triggers qemu_flush_queued_packets() in tulip_write() is sufficient. This to make sure net core can restore the receiving. Btw, what's the point of checking rx_frame_len here? Thanks > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
+-- On Tue, 17 Mar 2020, Jason Wang wrote --+
| > +-- On Fri, 6 Mar 2020, Stefan Hajnoczi wrote --+
| > | > +static int
| > | > +tulip_can_receive(NetClientState *nc)
| > | > +{
| > | > + TULIPState *s = qemu_get_nic_opaque(nc);
| > | > +
| > | > + if (s->rx_frame_len || tulip_rx_stopped(s)) {
| > | > + return false;
| > | > + }
|
| Btw, what's the point of checking rx_frame_len here?
tulip_can_recive() is called from tulip_receive(). IIUC non zero(0)
'rx_frame_len' hints that s->rs_frame[] buffer still has unread data bytes and
it can not receive new bytes. The check was earlier in tulip_receive().
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 2020/3/17 下午6:49, P J P wrote: > +-- On Tue, 17 Mar 2020, Jason Wang wrote --+ > | > +-- On Fri, 6 Mar 2020, Stefan Hajnoczi wrote --+ > | > | > +static int > | > | > +tulip_can_receive(NetClientState *nc) > | > | > +{ > | > | > + TULIPState *s = qemu_get_nic_opaque(nc); > | > | > + > | > | > + if (s->rx_frame_len || tulip_rx_stopped(s)) { > | > | > + return false; > | > | > + } > | > | Btw, what's the point of checking rx_frame_len here? > > tulip_can_recive() is called from tulip_receive(). IIUC non zero(0) > 'rx_frame_len' hints that s->rs_frame[] buffer still has unread data bytes and > it can not receive new bytes. The check was earlier in tulip_receive(). Right, so need to make sure qemu_flush_ququed_packets() was called when rx_frame_len is zero. Thanks > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Hello Jason, +-- On Wed, 18 Mar 2020, Jason Wang wrote --+ | Right, so need to make sure qemu_flush_ququed_packets() was called when | rx_frame_len is zero. Sent patch v4, with this call. Please see when you've time. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff --git a/hw/net/tulip.c b/hw/net/tulip.c index fbe40095da..757f12c710 100644 --- a/hw/net/tulip.c +++ b/hw/net/tulip.c @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState *s, const uint8_t *addr) return ret; } +static int +tulip_can_receive(NetClientState *nc) +{ + TULIPState *s = qemu_get_nic_opaque(nc); + + if (s->rx_frame_len || tulip_rx_stopped(s)) { + return false; + } + + return true; +} + static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size) { struct tulip_descriptor desc; @@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size) trace_tulip_receive(buf, size); if (size < 14 || size > sizeof(s->rx_frame) - 4 - || s->rx_frame_len || tulip_rx_stopped(s)) { + || !tulip_can_receive(s->nic->ncs)) { return 0; } @@ -288,6 +300,7 @@ static NetClientInfo net_tulip_info = { .type = NET_CLIENT_DRIVER_NIC, .size = sizeof(NICState), .receive = tulip_receive_nc, + .can_receive = tulip_can_receive, }; static const char *tulip_reg_name(const hwaddr addr)