urndis0: urndis_decap invalid buffer len 1 < minimum header 44

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

urndis0: urndis_decap invalid buffer len 1 < minimum header 44

Artturi Alm
Hi,

spam in the subject does make ttyC0 unusable in practice with urndis(4).
I don't like repeating myself, so all i provide is the link where this
behaviour requiring while (len > 1) is explained:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets

I'm running the minimal diff below on amd64 running 6.4.

-Artturi


diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
index f6b3c7bad9d..c5b4bc4ed3c 100644
--- a/sys/dev/usb/if_urndis.c
+++ b/sys/dev/usb/if_urndis.c
@@ -826,7 +826,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
  ifp = GET_IFP(sc);
  offset = 0;
 
- while (len > 0) {
+ while (len > 1) {
  msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset);
  m = c->sc_mbuf;
 
@@ -839,7 +839,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
     DEVNAME(sc),
     len,
     sizeof(*msg));
- return;
+ break;
  }
 
  DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) "
@@ -859,14 +859,14 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
     DEVNAME(sc),
     letoh32(msg->rm_type),
     REMOTE_NDIS_PACKET_MSG);
- return;
+ break;
  }
  if (letoh32(msg->rm_len) < sizeof(*msg)) {
  printf("%s: urndis_decap invalid msg len %u < %zu\n",
     DEVNAME(sc),
     letoh32(msg->rm_len),
     sizeof(*msg));
- return;
+ break;
  }
  if (letoh32(msg->rm_len) > len) {
  printf("%s: urndis_decap invalid msg len %u > buffer "
@@ -874,7 +874,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
     DEVNAME(sc),
     letoh32(msg->rm_len),
     len);
- return;
+ break;
  }
 
  if (letoh32(msg->rm_dataoffset) +
@@ -889,7 +889,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
     letoh32(msg->rm_dataoffset) +
     letoh32(msg->rm_datalen) + RNDIS_HEADER_OFFSET,
     letoh32(msg->rm_len));
- return;
+ break;
  }
 
  if (letoh32(msg->rm_datalen) < sizeof(struct ether_header)) {
@@ -899,7 +899,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
     DEVNAME(sc),
     letoh32(msg->rm_datalen),
     sizeof(struct ether_header)));
- return;
+ break;
  }
 
  memcpy(mtod(m, char*),
@@ -916,6 +916,8 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
  offset += letoh32(msg->rm_len);
  len -= letoh32(msg->rm_len);
  }
+ if (ml_empty(&ml))
+ return;
 
  s = splnet();
  if_input(ifp, &ml);

Reply | Threaded
Open this post in threaded view
|

Re: urndis0: urndis_decap invalid buffer len 1 < minimum header 44

Artturi Alm
On Wed, Oct 31, 2018 at 02:55:20AM +0200, Artturi Alm wrote:
> Hi,
>
> spam in the subject does make ttyC0 unusable in practice with urndis(4).
> I don't like repeating myself, so all i provide is the link where this
> behaviour requiring while (len > 1) is explained:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets
>
> I'm running the minimal diff below on amd64 running 6.4.
>

ping pong?

i've already done $ rm ~/diffs/obsd-*.diff
so currently there's no need to worry about me going after any other
bugs/changes relevant to me, even if this was fixed.

waste of time is, waste of time.
-Artturi


> -Artturi
>
>
> diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
> index f6b3c7bad9d..c5b4bc4ed3c 100644
> --- a/sys/dev/usb/if_urndis.c
> +++ b/sys/dev/usb/if_urndis.c
> @@ -826,7 +826,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>   ifp = GET_IFP(sc);
>   offset = 0;
>  
> - while (len > 0) {
> + while (len > 1) {
>   msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset);
>   m = c->sc_mbuf;
>  
> @@ -839,7 +839,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      len,
>      sizeof(*msg));
> - return;
> + break;
>   }
>  
>   DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) "
> @@ -859,14 +859,14 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_type),
>      REMOTE_NDIS_PACKET_MSG);
> - return;
> + break;
>   }
>   if (letoh32(msg->rm_len) < sizeof(*msg)) {
>   printf("%s: urndis_decap invalid msg len %u < %zu\n",
>      DEVNAME(sc),
>      letoh32(msg->rm_len),
>      sizeof(*msg));
> - return;
> + break;
>   }
>   if (letoh32(msg->rm_len) > len) {
>   printf("%s: urndis_decap invalid msg len %u > buffer "
> @@ -874,7 +874,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_len),
>      len);
> - return;
> + break;
>   }
>  
>   if (letoh32(msg->rm_dataoffset) +
> @@ -889,7 +889,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      letoh32(msg->rm_dataoffset) +
>      letoh32(msg->rm_datalen) + RNDIS_HEADER_OFFSET,
>      letoh32(msg->rm_len));
> - return;
> + break;
>   }
>  
>   if (letoh32(msg->rm_datalen) < sizeof(struct ether_header)) {
> @@ -899,7 +899,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_datalen),
>      sizeof(struct ether_header)));
> - return;
> + break;
>   }
>  
>   memcpy(mtod(m, char*),
> @@ -916,6 +916,8 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>   offset += letoh32(msg->rm_len);
>   len -= letoh32(msg->rm_len);
>   }
> + if (ml_empty(&ml))
> + return;
>  
>   s = splnet();
>   if_input(ifp, &ml);

Reply | Threaded
Open this post in threaded view
|

Re: urndis0: urndis_decap invalid buffer len 1 < minimum header 44

Martin Pieuchot
In reply to this post by Artturi Alm
On 31/10/18(Wed) 02:55, Artturi Alm wrote:
> Hi,
>
> spam in the subject does make ttyC0 unusable in practice with urndis(4).
> I don't like repeating myself, so all i provide is the link where this
> behaviour requiring while (len > 1) is explained:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets

Could you explain in words why we need this change?  What does it solve?

>
> I'm running the minimal diff below on amd64 running 6.4.
>
> -Artturi
>
>
> diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
> index f6b3c7bad9d..c5b4bc4ed3c 100644
> --- a/sys/dev/usb/if_urndis.c
> +++ b/sys/dev/usb/if_urndis.c
> @@ -826,7 +826,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>   ifp = GET_IFP(sc);
>   offset = 0;
>  
> - while (len > 0) {
> + while (len > 1) {
>   msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset);
>   m = c->sc_mbuf;
>  
> @@ -839,7 +839,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      len,
>      sizeof(*msg));
> - return;
> + break;
>   }
>  
>   DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) "
> @@ -859,14 +859,14 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_type),
>      REMOTE_NDIS_PACKET_MSG);
> - return;
> + break;
>   }
>   if (letoh32(msg->rm_len) < sizeof(*msg)) {
>   printf("%s: urndis_decap invalid msg len %u < %zu\n",
>      DEVNAME(sc),
>      letoh32(msg->rm_len),
>      sizeof(*msg));
> - return;
> + break;
>   }
>   if (letoh32(msg->rm_len) > len) {
>   printf("%s: urndis_decap invalid msg len %u > buffer "
> @@ -874,7 +874,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_len),
>      len);
> - return;
> + break;
>   }
>  
>   if (letoh32(msg->rm_dataoffset) +
> @@ -889,7 +889,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      letoh32(msg->rm_dataoffset) +
>      letoh32(msg->rm_datalen) + RNDIS_HEADER_OFFSET,
>      letoh32(msg->rm_len));
> - return;
> + break;
>   }
>  
>   if (letoh32(msg->rm_datalen) < sizeof(struct ether_header)) {
> @@ -899,7 +899,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>      DEVNAME(sc),
>      letoh32(msg->rm_datalen),
>      sizeof(struct ether_header)));
> - return;
> + break;
>   }
>  
>   memcpy(mtod(m, char*),
> @@ -916,6 +916,8 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
>   offset += letoh32(msg->rm_len);
>   len -= letoh32(msg->rm_len);
>   }
> + if (ml_empty(&ml))
> + return;
>  
>   s = splnet();
>   if_input(ifp, &ml);
>

Reply | Threaded
Open this post in threaded view
|

Re: urndis0: urndis_decap invalid buffer len 1 < minimum header 44

Artturi Alm
On Tue, Jan 22, 2019 at 10:22:39AM -0200, Martin Pieuchot wrote:
> On 31/10/18(Wed) 02:55, Artturi Alm wrote:
> > Hi,
> >
> > spam in the subject does make ttyC0 unusable in practice with urndis(4).
> > I don't like repeating myself, so all i provide is the link where this
> > behaviour requiring while (len > 1) is explained:
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets
>
> Could you explain in words why we need this change?  What does it solve?

If the incoming packet size is (length % 64(=wMaxPacketSize) == 0),
Linux does lenght++ to allow us to force short reads[0], they call this
behaviour as "strict CDC-Ether conformance", our cdce(4) already does
the same[1].
And as-is without s/return/break/, valid packet(s) would be lost before
the 'zlp' at the end of it, even if the loop condition was fixed alone.

does that make sense?
-Artturi

[0]:
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/u_ether.c#L578
[1]:
https://github.com/openbsd/src/blame/master/sys/dev/usb/if_cdce.c#L745

>
> >
> > I'm running the minimal diff below on amd64 running 6.4.
> >
> > -Artturi
> >
> >
> > diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
> > index f6b3c7bad9d..c5b4bc4ed3c 100644
> > --- a/sys/dev/usb/if_urndis.c
> > +++ b/sys/dev/usb/if_urndis.c
> > @@ -826,7 +826,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >   ifp = GET_IFP(sc);
> >   offset = 0;
> >  
> > - while (len > 0) {
> > + while (len > 1) {
> >   msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset);
> >   m = c->sc_mbuf;
> >  
> > @@ -839,7 +839,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >      DEVNAME(sc),
> >      len,
> >      sizeof(*msg));
> > - return;
> > + break;
> >   }
> >  
> >   DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) "
> > @@ -859,14 +859,14 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >      DEVNAME(sc),
> >      letoh32(msg->rm_type),
> >      REMOTE_NDIS_PACKET_MSG);
> > - return;
> > + break;
> >   }
> >   if (letoh32(msg->rm_len) < sizeof(*msg)) {
> >   printf("%s: urndis_decap invalid msg len %u < %zu\n",
> >      DEVNAME(sc),
> >      letoh32(msg->rm_len),
> >      sizeof(*msg));
> > - return;
> > + break;
> >   }
> >   if (letoh32(msg->rm_len) > len) {
> >   printf("%s: urndis_decap invalid msg len %u > buffer "
> > @@ -874,7 +874,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >      DEVNAME(sc),
> >      letoh32(msg->rm_len),
> >      len);
> > - return;
> > + break;
> >   }
> >  
> >   if (letoh32(msg->rm_dataoffset) +
> > @@ -889,7 +889,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >      letoh32(msg->rm_dataoffset) +
> >      letoh32(msg->rm_datalen) + RNDIS_HEADER_OFFSET,
> >      letoh32(msg->rm_len));
> > - return;
> > + break;
> >   }
> >  
> >   if (letoh32(msg->rm_datalen) < sizeof(struct ether_header)) {
> > @@ -899,7 +899,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >      DEVNAME(sc),
> >      letoh32(msg->rm_datalen),
> >      sizeof(struct ether_header)));
> > - return;
> > + break;
> >   }
> >  
> >   memcpy(mtod(m, char*),
> > @@ -916,6 +916,8 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain *c, u_int32_t len)
> >   offset += letoh32(msg->rm_len);
> >   len -= letoh32(msg->rm_len);
> >   }
> > + if (ml_empty(&ml))
> > + return;
> >  
> >   s = splnet();
> >   if_input(ifp, &ml);
> >