| Message ID | 20100712174823.GA11411@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 2010-07-12 at 20:48 +0300, Michael S. Tsirkin wrote: > 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 <mst@redhat.com> > --- > > Anthony, Alex, please review. Looks fine to me. Acked-by: Alex Williamson <alex.williamson@redhat.com> > hw/e1000.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/hw/e1000.c b/hw/e1000.c > index 0da65f9..70aba11 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -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");
On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > 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<mst@redhat.com> > 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 > --- > > Anthony, Alex, please review. > > hw/e1000.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/hw/e1000.c b/hw/e1000.c > index 0da65f9..70aba11 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -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"); >
On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > >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<mst@redhat.com> > > 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? > Regards, > > Anthony Liguori > > >--- > > > >Anthony, Alex, please review. > > > > hw/e1000.c | 3 +-- > > 1 files changed, 1 insertions(+), 2 deletions(-) > > > >diff --git a/hw/e1000.c b/hw/e1000.c > >index 0da65f9..70aba11 100644 > >--- a/hw/e1000.c > >+++ b/hw/e1000.c > >@@ -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");
On 07/12/2010 04:30 PM, Michael S. Tsirkin wrote: > On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > >> On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: >> >>> 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<mst@redhat.com> >>> >> 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. Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori > >> Regards, >> >> Anthony Liguori >> >> >>> --- >>> >>> Anthony, Alex, please review. >>> >>> hw/e1000.c | 3 +-- >>> 1 files changed, 1 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/e1000.c b/hw/e1000.c >>> index 0da65f9..70aba11 100644 >>> --- a/hw/e1000.c >>> +++ b/hw/e1000.c >>> @@ -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"); >>>
On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > >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<mst@redhat.com> > > 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? > >--- > > > >Anthony, Alex, please review. > > > > hw/e1000.c | 3 +-- > > 1 files changed, 1 insertions(+), 2 deletions(-) > > > >diff --git a/hw/e1000.c b/hw/e1000.c > >index 0da65f9..70aba11 100644 > >--- a/hw/e1000.c > >+++ b/hw/e1000.c > >@@ -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");
On 07/12/2010 05:42 PM, Michael S. Tsirkin wrote: > On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > >> On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: >> >>> 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<mst@redhat.com> >>> >> 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? Regards, Anthony Liguori >>> --- >>> >>> Anthony, Alex, please review. >>> >>> hw/e1000.c | 3 +-- >>> 1 files changed, 1 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/e1000.c b/hw/e1000.c >>> index 0da65f9..70aba11 100644 >>> --- a/hw/e1000.c >>> +++ b/hw/e1000.c >>> @@ -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"); >>>
On Mon, Jul 12, 2010 at 06:00:20PM -0500, Anthony Liguori wrote: > On 07/12/2010 05:42 PM, Michael S. Tsirkin wrote: > >On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > >>On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > >>>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<mst@redhat.com> > >>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? > 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. > Regards, > > Anthony Liguori > > >>>--- > >>> > >>>Anthony, Alex, please review. > >>> > >>> hw/e1000.c | 3 +-- > >>> 1 files changed, 1 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/e1000.c b/hw/e1000.c > >>>index 0da65f9..70aba11 100644 > >>>--- a/hw/e1000.c > >>>+++ b/hw/e1000.c > >>>@@ -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"); > -- Gleb.
On Tue, Jul 13, 2010 at 09:35:49AM +0300, Gleb Natapov wrote: > On Mon, Jul 12, 2010 at 06:00:20PM -0500, Anthony Liguori wrote: > > On 07/12/2010 05:42 PM, Michael S. Tsirkin wrote: > > >On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > > >>On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > > >>>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<mst@redhat.com> > > >>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? > > > 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? > > Regards, > > > > Anthony Liguori > > > > >>>--- > > >>> > > >>>Anthony, Alex, please review. > > >>> > > >>> hw/e1000.c | 3 +-- > > >>> 1 files changed, 1 insertions(+), 2 deletions(-) > > >>> > > >>>diff --git a/hw/e1000.c b/hw/e1000.c > > >>>index 0da65f9..70aba11 100644 > > >>>--- a/hw/e1000.c > > >>>+++ b/hw/e1000.c > > >>>@@ -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"); > > > > -- > Gleb.
On Tue, Jul 13, 2010 at 02:11:10PM +0300, Michael S. Tsirkin wrote: > On Tue, Jul 13, 2010 at 09:35:49AM +0300, Gleb Natapov wrote: > > On Mon, Jul 12, 2010 at 06:00:20PM -0500, Anthony Liguori wrote: > > > On 07/12/2010 05:42 PM, Michael S. Tsirkin wrote: > > > >On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > > > >>On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > > > >>>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<mst@redhat.com> > > > >>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? > > > > > 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.
diff --git a/hw/e1000.c b/hw/e1000.c index 0da65f9..70aba11 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -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");
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 <mst@redhat.com> --- Anthony, Alex, please review. hw/e1000.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)