e1000: fix access 4 bytes beyond buffer end
RFC PATCH 1/1 e1000: fix access 4 bytes beyond buffer end
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
hw/e1000.c +1 -2
@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT] && s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);
@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Signature · 1 line
1.7.2.rc0.14.g41c1c
Quoted 8 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin <[email protected]>---
Anthony, Alex, please review.
Looks fine to me.
Quoted 24 lines
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT] && s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Quoted 6 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>
I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Regards,
Anthony Liguori
Quoted 29 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Quoted 9 lines · Anthony Liguori
Quoted 5 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
It's the buffer guest allocated, and we leave it untouched. How does this leak data?
Quoted 32 lines
Regards,
Anthony Liguori
Quoted 28 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Anthony Liguori
Reviewed-byQuoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Anthony Liguori
Quoted 4 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
It's the buffer guest allocated, and we leave it untouched. How does this leak data?
Sorry, you're right.
Regards,
Anthony Liguori
Quoted 35 lines
Quoted 35 lines
Regards,
Anthony Liguori
Quoted 29 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Quoted 5 lines · Anthony Liguori
Quoted 4 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Quoted 8 lines
Quoted 1 line
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Regards,
Anthony Liguori
I am guessing there's no chance guest actually looks
at this data, otherwise it won't match and we'd get errors, right?
Quoted 28 lines
Quoted 28 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Quoted 13 lines · Michael S. Tsirkin
Quoted 11 lines · Anthony Liguori
Quoted 6 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Quoted 7 lines
Quoted 4 lines
Regards,
Anthony Liguori
I am guessing there's no chance guest actually looks
at this data, otherwise it won't match and we'd get errors, right?
That's my assumption too. Although I believe there are some known issues with e1000 and certain versions of Windows and the Microsoft built-in driver. Maybe this is why those drivers don't work and the Intel drivers do?
Regards,
Anthony Liguori
Quoted 29 lines
Quoted 29 lines
Quoted 29 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +1
-2@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) } rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
Quoted 13 lines · Anthony Liguori
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Anthony Liguori
Quoted 5 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Regards,
Quoted 9 lines
Quoted 3 lines
Quoted 1 line
Anthony Liguori
I am guessing there's no chance guest actually looks
at this data, otherwise it won't match and we'd get errors, right?That's my assumption too. Although I believe there are some known issues with e1000 and certain versions of Windows and the Microsoft built-in driver. Maybe this is why those drivers don't work and the Intel drivers do?
At least one known issue with Windows drivers to me is that they sometimes (on resume from S4 at least) enable interrupts before setup irq routing, so if interrupt is generated in the wrong time it hangs the guest. I guess it works on real HW for them because line speed negotiation takes non-zero time.
Quoted 18 lines
Regards,
Anthony Liguori
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +0
-0@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) }
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
hw/e1000.c +0
-1rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)Quoted 8 lines
Quoted 8 lines
hw/e1000.c e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) · line 668
Quoted 8 lines
hw/e1000.c +1
-1if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );
--
Gleb.
Quoted 23 lines · Gleb Natapov
Quoted 13 lines · Anthony Liguori
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Anthony Liguori
Quoted 5 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Regards,
Quoted 9 lines
Quoted 3 lines
Quoted 1 line
Anthony Liguori
I am guessing there's no chance guest actually looks
at this data, otherwise it won't match and we'd get errors, right?That's my assumption too. Although I believe there are some known issues with e1000 and certain versions of Windows and the Microsoft built-in driver. Maybe this is why those drivers don't work and the Intel drivers do?
Quoted 5 lines
At least one known issue with Windows drivers to me is that they sometimes (on resume from S4 at least) enable interrupts before setup irq routing, so if interrupt is generated in the wrong time it hangs the guest. I guess it works on real HW for them because line speed negotiation takes non-zero time.
I guess we could work around this. Is there a bz?
Quoted 36 lines
Quoted 18 lines
Regards,
Anthony Liguori
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
---
Anthony, Alex, please review.
hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)hw/e1000.c +0
-0@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) }Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
hw/e1000.c +0
-1rdh_start = s->mac_reg[RDH ]; - size +=4 ;// for the header do { if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { set_ics(s, 0, E1000_ICS_RXO);@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)Quoted 8 lines
Quoted 8 lines
hw/e1000.c e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) · line 668
Quoted 8 lines
hw/e1000.c +1
-1if (desc.buffer_addr) {cpu_physical_memory_write (le64_to_cpu (desc.buffer_addr), (void *)(buf + vlan_offset), size); - desc.length =cpu_to_le16 (size); + desc.length =cpu_to_le16 (size +4 /* for FCS */ ); desc.status |=E1000_RXD_STAT_EOP |E1000_RXD_STAT_IXSM ; }else // as per intel docs; skip descriptors with null buf addr DBGOUT (RX ,"Null RX descriptor!!\n" );--
Gleb.
Quoted 24 lines · Michael S. Tsirkin
Quoted 23 lines · Gleb Natapov
Quoted 22 lines · Anthony Liguori
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Anthony Liguori
Quoted 5 lines · Michael S. Tsirkin
We do range check for size, and get size as buffer, but copy size + 4 bytes (4 is for FCS). Let's copy size bytes but put size + 4 in length.
Signed-off-by Michael S. Tsirkin<[email protected]>Quoted 8 lines
Quoted 6 lines
I think I'd feel slightly better if we zero'd out the FCS before writing it to the guest. It is potentially a data leak.
Regards,
Anthony Liguori
I am guessing there's no chance guest actually looks
at this data, otherwise it won't match and we'd get errors, right?That's my assumption too. Although I believe there are some known issues with e1000 and certain versions of Windows and the Microsoft built-in driver. Maybe this is why those drivers don't work and the Intel drivers do?
Quoted 8 lines
Quoted 5 lines
At least one known issue with Windows drivers to me is that they sometimes (on resume from S4 at least) enable interrupts before setup irq routing, so if interrupt is generated in the wrong time it hangs the guest. I guess it works on real HW for them because line speed negotiation takes non-zero time.
I guess we could work around this. Is there a bz?
BZ where? We do not support e1000 with Windows guests.
--
Gleb.