Re: urndis(4) fix

2018-09-23 Thread Artturi Alm 
On Mon, Sep 24, 2018 at 03:34:59AM +0300, Artturi Alm wrote:
> On Sat, Nov 29, 2014 at 09:42:51PM +0100, Mark Kettenis wrote:
> > Recent Oracle SPARC machines have a USB gadget to talk to the Service
> > Processor (ILOM).  This gadget supports both RNDIS and CDC Ethernet.
> > The RNDIS bits uncovered a bug in urndis(4).  When urndis_ctrl_set()
> > sets up the REMOTE_NDIS_SET_MSG command it sets up msg->rm_infobuflen,
> > it subsequently overwrites its contents.  Apparently many RNDIS
> > devices don't care, but this hardware sends a response back that
> > indicates the command wasn't accepted.  Diff below fixes this problem.
> > It matches what Linux does.
> > 
> > Unfortunately, this isn't enough to make the gadget work in RNDIS mode.
> > 
> > I'd appreciate it if people using urndis(4) could test this diff.
> > 
> 
> Hi,
> 
> just found your mail, i think urndis(4) is not entirely written with
> these in mind:
>   defined(__STRICT_ALIGNMENT) || _BYTE_ORDER != _LITTLE_ENDIAN
> 
> did you ever try with a diff like below?
> 
> -Artturi
> 

sorry for the 'spam', on a second read of your message, i think it might
be just more strict than ie. android, not allowing the _SET_MSG from
urndis_attach(), because it's "out-of-order" with regards to msg sent
by urndis_ctrl_init(). if it's not in the "spec", atleast MS[0] does
show things done differently, == init first before setting data packet
filter.

-Artturi

[0]:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/example-connectionless--802-3--initialization-sequence

Re: urndis(4) fix

2018-09-23 Thread Artturi Alm 
On Sat, Nov 29, 2014 at 09:42:51PM +0100, Mark Kettenis wrote:
> Recent Oracle SPARC machines have a USB gadget to talk to the Service
> Processor (ILOM).  This gadget supports both RNDIS and CDC Ethernet.
> The RNDIS bits uncovered a bug in urndis(4).  When urndis_ctrl_set()
> sets up the REMOTE_NDIS_SET_MSG command it sets up msg->rm_infobuflen,
> it subsequently overwrites its contents.  Apparently many RNDIS
> devices don't care, but this hardware sends a response back that
> indicates the command wasn't accepted.  Diff below fixes this problem.
> It matches what Linux does.
> 
> Unfortunately, this isn't enough to make the gadget work in RNDIS mode.
> 
> I'd appreciate it if people using urndis(4) could test this diff.
> 

Hi,

just found your mail, i think urndis(4) is not entirely written with
these in mind:
defined(__STRICT_ALIGNMENT) || _BYTE_ORDER != _LITTLE_ENDIAN

did you ever try with a diff like below?

-Artturi


diff --git sys/dev/rndis.h sys/dev/rndis.h
index 140793990e9..7036d198d4e 100644
--- sys/dev/rndis.h
+++ sys/dev/rndis.h
@@ -98,7 +98,7 @@
 struct rndis_msghdr {
uint32_trm_type;
uint32_trm_len;
-};
+} __packed;
 
 /*
  * RNDIS data message
@@ -118,7 +118,7 @@ struct rndis_packet_msg {
uint32_trm_pktinfolen;
uint32_trm_vchandle;
uint32_trm_reserved;
-};
+} __packed;
 
 /* Per-packet-info for RNDIS data message */
 struct rndis_pktinfo {
@@ -126,7 +126,7 @@ struct rndis_pktinfo {
uint32_trm_type;/* NDIS_PKTINFO_TYPE_ */
uint32_trm_pktinfooffset;
uint8_t rm_data[0];
-};
+} __packed;
 
 #define NDIS_PKTINFO_TYPE_CSUM 0
 #define NDIS_PKTINFO_TYPE_IPSEC1
@@ -149,7 +149,7 @@ struct rndis_comp_hdr {
uint32_trm_len;
uint32_trm_rid;
uint32_trm_status;
-};
+} __packed;
 
 /* Initialize the device. */
 #define REMOTE_NDIS_INITIALIZE_MSG 0x0002
@@ -162,7 +162,7 @@ struct rndis_init_req {
uint32_trm_ver_major;
uint32_trm_ver_minor;
uint32_trm_max_xfersz;
-};
+} __packed;
 
 struct rndis_init_comp {
uint32_trm_type;
@@ -178,7 +178,7 @@ struct rndis_init_comp {
uint32_trm_align;
uint32_trm_aflistoffset;
uint32_trm_aflistsz;
-};
+} __packed;
 
 /* Halt the device.  No response sent. */
 #define REMOTE_NDIS_HALT_MSG   0x0003
@@ -187,7 +187,7 @@ struct rndis_halt_req {
uint32_trm_type;
uint32_trm_len;
uint32_trm_rid;
-};
+} __packed;
 
 /* Send a query object. */
 #define REMOTE_NDIS_QUERY_MSG  0x0004
@@ -201,7 +201,7 @@ struct rndis_query_req {
uint32_trm_infobuflen;
uint32_trm_infobufoffset;
uint32_trm_devicevchdl;
-};
+} __packed;
 
 struct rndis_query_comp {
uint32_trm_type;
@@ -210,7 +210,7 @@ struct rndis_query_comp {
uint32_trm_status;
uint32_trm_infobuflen;
uint32_trm_infobufoffset;
-};
+} __packed;
 
 /* Send a set object request. */
 #define REMOTE_NDIS_SET_MSG0x0005
@@ -224,14 +224,14 @@ struct rndis_set_req {
uint32_trm_infobuflen;
uint32_trm_infobufoffset;
uint32_trm_devicevchdl;
-};
+} __packed;
 
 struct rndis_set_comp {
uint32_trm_type;
uint32_trm_len;
uint32_trm_rid;
uint32_trm_status;
-};
+} __packed;
 
 /* Parameter used by OID_GEN_RNDIS_CONFIG_PARAMETER. */
 #define REMOTE_NDIS_SET_PARAM_NUMERIC  0x
@@ -243,7 +243,7 @@ struct rndis_set_parameter {
uint32_trm_type;
uint32_trm_valueoffset;
uint32_trm_valuelen;
-};
+} __packed;
 
 /* Perform a soft reset on the device. */
 #define REMOTE_NDIS_RESET_MSG  0x0006
@@ -253,14 +253,14 @@ struct rndis_reset_req {
uint32_trm_type;
uint32_trm_len;
uint32_trm_rid;
-};
+} __packed;
 
 struct rndis_reset_comp {
uint32_trm_type;
uint32_trm_len;
uint32_trm_status;
uint32_trm_adrreset;
-};
+} __packed;
 
 /* 802.3 link-state or undefined message error.  Sent by device. */
 #define REMOTE_NDIS_INDICATE_STATUS_MSG0x0007
@@ -272,7 +272,7 @@ struct rndis_status_msg {
uint32_trm_stbuflen;
uint32_trm_stbufoffset;
/* rndis_diag_info */
-};
+} __packed;
 
 /*
  * Immediately after rndis_status_msg.rm_stbufoffset, if a control
@@ -282,7 +282,7 @@ struct rndis_status_msg {
 struct rndis_diag_info {
uint32_trm_diagstatus;
uint32_trm_erroffset;
-};
+} __packed;
 
 /* Keepalive messsage.  May be sent by device. */
 #define REMOTE_NDIS_KEEPALIVE_MSG

Re: urndis(4) fix

2014-11-30 Thread Jonathan Armani 
On Sat, Nov 29, 2014 at 09:42:51PM +0100, Mark Kettenis wrote:
 Recent Oracle SPARC machines have a USB gadget to talk to the Service
 Processor (ILOM).  This gadget supports both RNDIS and CDC Ethernet.
 The RNDIS bits uncovered a bug in urndis(4).  When urndis_ctrl_set()
 sets up the REMOTE_NDIS_SET_MSG command it sets up msg-rm_infobuflen,
 it subsequently overwrites its contents.  Apparently many RNDIS
 devices don't care, but this hardware sends a response back that
 indicates the command wasn't accepted.  Diff below fixes this problem.
 It matches what Linux does.
 
 Unfortunately, this isn't enough to make the gadget work in RNDIS mode.
 
 I'd appreciate it if people using urndis(4) could test this diff.
 

Makes sense, ok armani@ (over urndis)

urndis(4) fix

2014-11-29 Thread Mark Kettenis 
Recent Oracle SPARC machines have a USB gadget to talk to the Service
Processor (ILOM).  This gadget supports both RNDIS and CDC Ethernet.
The RNDIS bits uncovered a bug in urndis(4).  When urndis_ctrl_set()
sets up the REMOTE_NDIS_SET_MSG command it sets up msg-rm_infobuflen,
it subsequently overwrites its contents.  Apparently many RNDIS
devices don't care, but this hardware sends a response back that
indicates the command wasn't accepted.  Diff below fixes this problem.
It matches what Linux does.

Unfortunately, this isn't enough to make the gadget work in RNDIS mode.

I'd appreciate it if people using urndis(4) could test this diff.


Index: if_urndis.c
===
RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
retrieving revision 1.49
diff -u -p -r1.49 if_urndis.c
--- if_urndis.c 13 Jul 2014 15:52:49 -  1.49
+++ if_urndis.c 29 Nov 2014 20:30:24 -
@@ -554,7 +554,7 @@ urndis_ctrl_set(struct urndis_softc *sc,
msg-rm_infobuflen = htole32(len);
if (len != 0) {
msg-rm_infobufoffset = htole32(20);
-   memcpy((char*)msg + 20, buf, len);
+   memcpy((char*)msg + 28, buf, len);
} else
msg-rm_infobufoffset = 0;
msg-rm_devicevchdl = 0;
@@ -570,7 +570,7 @@ urndis_ctrl_set(struct urndis_softc *sc,
letoh32(msg-rm_infobufoffset),
letoh32(msg-rm_devicevchdl)));
 
-   rval = urndis_ctrl_send(sc, msg, sizeof(*msg));
+   rval = urndis_ctrl_send(sc, msg, sizeof(*msg) + len);
free(msg, M_TEMP, 0);
 
if (rval != RNDIS_STATUS_SUCCESS) {

urndis(4): fix charging on my nokia c2-01

2013-12-02 Thread Paul Irofti 
Could people doing actual networking with urndis(4) please test the
following diff and report if any regressions are encountered?

I plan on commiting it by the end of the week if I don't hear anything
bad in regards to the patch.


Index: if_urndis.c
===
RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
retrieving revision 1.44
diff -u -p -r1.44 if_urndis.c
--- if_urndis.c 21 Nov 2013 14:08:05 -  1.44
+++ if_urndis.c 28 Nov 2013 10:22:42 -
@@ -1363,6 +1363,7 @@ urndis_attach(struct device *parent, str
sc = (void *)self;
uaa = aux;
 
+   sc-sc_attached = 0;
sc-sc_udev = uaa-device;
id = usbd_get_interface_descriptor(uaa-iface);
sc-sc_ifaceno_ctl = id-bInterfaceNumber;
@@ -1447,14 +1448,11 @@ urndis_attach(struct device *parent, str
 
IFQ_SET_READY(ifp-if_snd);
 
-   urndis_init(sc);
-
s = splnet();
 
if (urndis_ctrl_query(sc, OID_802_3_PERMANENT_ADDRESS, NULL, 0,
buf, bufsz) != RNDIS_STATUS_SUCCESS) {
printf(: unable to get hardware address\n);
-   urndis_stop(sc);
splx(s);
return;
}
@@ -1466,7 +1464,6 @@ urndis_attach(struct device *parent, str
} else {
printf(, invalid address\n);
free(buf, M_TEMP);
-   urndis_stop(sc);
splx(s);
return;
}
@@ -1478,7 +1475,6 @@ urndis_attach(struct device *parent, str
if (urndis_ctrl_set(sc, OID_GEN_CURRENT_PACKET_FILTER, filter,
sizeof(filter)) != RNDIS_STATUS_SUCCESS) {
printf(%s: unable to set data filters\n, DEVNAME(sc));
-   urndis_stop(sc);
splx(s);
return;
}

Re: urndis(4): fix charging on my nokia c2-01

2013-12-02 Thread Paul Irofti 
On Tue, Dec 03, 2013 at 12:47:30AM +0200, Paul Irofti wrote:
 Could people doing actual networking with urndis(4) please test the
 following diff and report if any regressions are encountered?
 
 I plan on commiting it by the end of the week if I don't hear anything
 bad in regards to the patch.
 

To be more specific, this diff fixes a panic on detach when connecting
my phone to an OpenBSD machine.

The ctrl methods on attach only use control messages and don't need any
pipe besides the default pipe which is always present.

Thus the urndis_init() and, in turn, urndis_stop() funcalls are not
needed during attach.

Besides from that they're also poorly written as they can fail and that
is not checked nor reported. This driver is in really poor shape.

This diff just brings this awful driver closer to what other usb
ethernet drivers are doing.


Others diffs will probably follow in order to clean-up the current mess.


 Index: if_urndis.c
 ===
 RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
 retrieving revision 1.44
 diff -u -p -r1.44 if_urndis.c
 --- if_urndis.c   21 Nov 2013 14:08:05 -  1.44
 +++ if_urndis.c   28 Nov 2013 10:22:42 -
 @@ -1363,6 +1363,7 @@ urndis_attach(struct device *parent, str
   sc = (void *)self;
   uaa = aux;
  
 + sc-sc_attached = 0;
   sc-sc_udev = uaa-device;
   id = usbd_get_interface_descriptor(uaa-iface);
   sc-sc_ifaceno_ctl = id-bInterfaceNumber;
 @@ -1447,14 +1448,11 @@ urndis_attach(struct device *parent, str
  
   IFQ_SET_READY(ifp-if_snd);
  
 - urndis_init(sc);
 -
   s = splnet();
  
   if (urndis_ctrl_query(sc, OID_802_3_PERMANENT_ADDRESS, NULL, 0,
   buf, bufsz) != RNDIS_STATUS_SUCCESS) {
   printf(: unable to get hardware address\n);
 - urndis_stop(sc);
   splx(s);
   return;
   }
 @@ -1466,7 +1464,6 @@ urndis_attach(struct device *parent, str
   } else {
   printf(, invalid address\n);
   free(buf, M_TEMP);
 - urndis_stop(sc);
   splx(s);
   return;
   }
 @@ -1478,7 +1475,6 @@ urndis_attach(struct device *parent, str
   if (urndis_ctrl_set(sc, OID_GEN_CURRENT_PACKET_FILTER, filter,
   sizeof(filter)) != RNDIS_STATUS_SUCCESS) {
   printf(%s: unable to set data filters\n, DEVNAME(sc));
 - urndis_stop(sc);
   splx(s);
   return;
   }

Re: urndis(4): fix charging on my nokia c2-01

2013-12-02 Thread Paul Irofti 
On Tue, Dec 03, 2013 at 01:07:17AM +0200, Paul Irofti wrote:
 On Tue, Dec 03, 2013 at 12:47:30AM +0200, Paul Irofti wrote:
  Could people doing actual networking with urndis(4) please test the
  following diff and report if any regressions are encountered?
  
  I plan on commiting it by the end of the week if I don't hear anything
  bad in regards to the patch.
  
 
 To be more specific, this diff fixes a panic on detach when connecting
 my phone to an OpenBSD machine.

Clarifying further, this is due to failure to setup a pipe in
urndis_ctrl_init() which results in a TIMEOUT on receive with
side-effects later on during detach due to assumptions regarding
the existence of interface hooks during dohooks on if_detach tear-down.

This results on dereferencing a NULL function pointer which triggers a
panic.

 
 The ctrl methods on attach only use control messages and don't need any
 pipe besides the default pipe which is always present.
 
 Thus the urndis_init() and, in turn, urndis_stop() funcalls are not
 needed during attach.
 
 Besides from that they're also poorly written as they can fail and that
 is not checked nor reported. This driver is in really poor shape.
 
 This diff just brings this awful driver closer to what other usb
 ethernet drivers are doing.
 
 
 Others diffs will probably follow in order to clean-up the current mess.
 
 
  Index: if_urndis.c
  ===
  RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
  retrieving revision 1.44
  diff -u -p -r1.44 if_urndis.c
  --- if_urndis.c 21 Nov 2013 14:08:05 -  1.44
  +++ if_urndis.c 28 Nov 2013 10:22:42 -
  @@ -1363,6 +1363,7 @@ urndis_attach(struct device *parent, str
  sc = (void *)self;
  uaa = aux;
   
  +   sc-sc_attached = 0;
  sc-sc_udev = uaa-device;
  id = usbd_get_interface_descriptor(uaa-iface);
  sc-sc_ifaceno_ctl = id-bInterfaceNumber;
  @@ -1447,14 +1448,11 @@ urndis_attach(struct device *parent, str
   
  IFQ_SET_READY(ifp-if_snd);
   
  -   urndis_init(sc);
  -
  s = splnet();
   
  if (urndis_ctrl_query(sc, OID_802_3_PERMANENT_ADDRESS, NULL, 0,
  buf, bufsz) != RNDIS_STATUS_SUCCESS) {
  printf(: unable to get hardware address\n);
  -   urndis_stop(sc);
  splx(s);
  return;
  }
  @@ -1466,7 +1464,6 @@ urndis_attach(struct device *parent, str
  } else {
  printf(, invalid address\n);
  free(buf, M_TEMP);
  -   urndis_stop(sc);
  splx(s);
  return;
  }
  @@ -1478,7 +1475,6 @@ urndis_attach(struct device *parent, str
  if (urndis_ctrl_set(sc, OID_GEN_CURRENT_PACKET_FILTER, filter,
  sizeof(filter)) != RNDIS_STATUS_SUCCESS) {
  printf(%s: unable to set data filters\n, DEVNAME(sc));
  -   urndis_stop(sc);
  splx(s);
  return;
  }