Re: umb(4) WIP diff and questions

2020-01-22 Thread Lee B
On Thu Jan 23, 2020 at 3:05 AM, Claudio Jeker wrote:
> On Thu, Jan 23, 2020 at 10:48:06AM +0900, Lee B wrote:
> >  
> > OK, the umb_softc part was straightforward enough, thanks. I'd like
> > some advice on how to handle the ifconfig(8) changes to accomodate
> > this though. I see the wifi code appears to use a separate ioctl pair
> > to handle the authentication credentials (WPA/PSK). Is this the right 
> > way to go, or have I missed an easier solution?
> > 
>
> 
> No that is the best way. Create a new ioctl to set the data. You can
> decide if you want to pass the data together ot split in two commands. I
> think a single ioctl has benefits for the driver but is more annoying to
> handle in ifconfig.
>

Great, thank you. I'll hopefully get some time to finish this over the 
weekend.

Lee.



Re: umb(4) WIP diff and questions

2020-01-22 Thread Claudio Jeker
On Thu, Jan 23, 2020 at 10:48:06AM +0900, Lee B wrote:
> On Tue Jan 14, 2020 at 5:59 PM, Claudio Jeker wrote:
> > 
> > Since the credentials should not be passed back to userland I would not
> > add them to struct umb_parameter but instead to struct umb_softc.
> > Then you don't need to use struct umb_parameter for the ioctl and
> > instead
> > could just pass the (utf16) string to the kernel.
> > Apart form that it looks good.
> >
> > 
> 
> OK, the umb_softc part was straightforward enough, thanks. I'd like
> some advice on how to handle the ifconfig(8) changes to accomodate
> this though. I see the wifi code appears to use a separate ioctl pair
> to handle the authentication credentials (WPA/PSK). Is this the right 
> way to go, or have I missed an easier solution?
> 

No that is the best way. Create a new ioctl to set the data. You can
decide if you want to pass the data together ot split in two commands. I
think a single ioctl has benefits for the driver but is more annoying to
handle in ifconfig.

-- 
:wq Claudio



Re: umb(4) WIP diff and questions

2020-01-22 Thread Lee B
On Tue Jan 14, 2020 at 5:59 PM, Claudio Jeker wrote:
> 
> Since the credentials should not be passed back to userland I would not
> add them to struct umb_parameter but instead to struct umb_softc.
> Then you don't need to use struct umb_parameter for the ioctl and
> instead
> could just pass the (utf16) string to the kernel.
> Apart form that it looks good.
>
> 

OK, the umb_softc part was straightforward enough, thanks. I'd like
some advice on how to handle the ifconfig(8) changes to accomodate
this though. I see the wifi code appears to use a separate ioctl pair
to handle the authentication credentials (WPA/PSK). Is this the right 
way to go, or have I missed an easier solution?

Lee.



Re: umb(4) WIP diff and questions

2020-01-22 Thread Ricardo Mestre
Hi,

Disclaimer: I don't have such hardware to test with or without the diff below,
but I think if we add this change in any shape or form then we should add this
as well otherwise we could bump into the vuln [0] that Ilja found on NetBSD
which could leak the credentials.

[0] 
https://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2020-001.txt.asc

Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.31
diff -u -p -u -r1.31 if_umb.c
--- if_umb.c26 Nov 2019 23:04:28 -  1.31
+++ if_umb.c21 Jan 2020 23:23:43 -
@@ -699,6 +699,8 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
usb_add_task(sc->sc_udev, >sc_umb_task);
break;
case SIOCGUMBINFO:
+   if ((error = suser(p)) != 0)
+   break;
error = copyout(>sc_info, ifr->ifr_data,
sizeof (sc->sc_info));
break;

On 12:40 Tue 14 Jan , Theo de Raadt wrote:
> Theo de Raadt  wrote:
> 
> > Stuart Henderson  wrote:
> > 
> > > On 2020/01/14 10:27, Theo de Raadt wrote:
> > > > Unfortunate part of this diff is that the password is (very
> > > > momentarily) visible with ps(1) in the root-run ifconfig argv[] array.
> > > > It's a tight race, but still it is visible.
> > > > 
> > > > People do run "sh /etc/netstart umb0" to activate the interface
> > > > during multiuser.
> > > > 
> > > > If the password is truly sensitive, it should be placed in a file,
> > > > and the ifconfig's extension should be changed to read the file.
> > > 
> > > That's not unique to umb though, it's been a problem forever with carp,
> > > pppoe and especially wlan interfaces.
> > 
> > When creating new versions of the same problem... that's a great time
> > to reconsider the old solutions.
> > 
> > > Another fix would be to accept
> > > ifconfig options on stdin, which is more convenient for quick runtime
> > > changes than two steps of writing to a file then pointing ifconfig at
> > > it, and changing netstart to use it would improve things for existing
> > > users without them needing to touch any config files.
> > 
> > I think using the shell is silly, because what if there are two such
> > options?  Then you can't seperate the stdin.
> > 
> > Additionally, most of the time when creating such temporary configuration,
> > the pattern is that if it works you'll want to apply it permanent.
> > 
> > Another thought is to create both versions of the option.  One on the
> > commandline, and one pointing at a file.
> > 
> > Channeling a conversation from 15 years ago: "How about wpakeyfile"
> 
> 
> Another consideration is... many of these passwords are locked to narrow
> usage cases, so does it really matter all that much?
> 



Re: umb(4) WIP diff and questions

2020-01-14 Thread Lee B
On Tue Jan 14, 2020 at 12:40 PM, Theo de Raadt wrote:
> > 
> > Channeling a conversation from 15 years ago: "How about wpakeyfile"
>
> 
>
> 
> Another consideration is... many of these passwords are locked to narrow
> usage cases, so does it really matter all that much?
>
> 

Right, seems like I should leave ifconfig(8) alone for the moment. I'll
carry on with umb and the suggestions from Stefan and Claudio though if
that's OK?




Re: umb(4) WIP diff and questions

2020-01-14 Thread Lee B
Hi Stefan,

On Tue Jan 14, 2020 at 2:40 PM, Stefan Sperling wrote:
>
<... lots of useful stuff ...>
> 

That was exactly the sort of thing I was looking for. Thanks!
It was seeing your device drivers presentation on Youtube a 
week or so ago that originally inspired me to get stuck in, so
thanks for that, too.

Lee.



Re: umb(4) WIP diff and questions

2020-01-14 Thread Abel Abraham Camarillo Ojeda
On Tue, Jan 14, 2020 at 5:11 PM Stefan Sperling  wrote:

> On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote:
> > Channeling a conversation from 15 years ago: "How about wpakeyfile"
>
> ifconfig wpakeyfile would be trivial to add if we really want it.
>

But how will hostname.if will work when using join in netstart, one would
need to:

# cat /etc/hostname.iwm0
join ssid1 wpakeyfile /etc/wpa/ssd1-wpa.key
join ssd2 wpakeyfile /etc/wpa/ssd2-wpa.key
[etc...]

?


>
> The downside is loss of unveil, here handled the same way as for the
> bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad
> practice even for an 'i' that should contain a path?
>
> diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src
> blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d
> file + sbin/ifconfig/ifconfig.8
> --- sbin/ifconfig/ifconfig.8
> +++ sbin/ifconfig/ifconfig.8
> @@ -940,6 +940,7 @@ will begin advertising as master.
>  .Op Cm wpaciphers Ar cipher,cipher,...
>  .Op Cm wpagroupcipher Ar cipher
>  .Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey
> +.Op Cm wpakeyfile Ar path
>  .Op Cm wpaprotos Ar proto,proto,...
>  .Ek
>  .nr nS 0
> @@ -990,6 +991,7 @@ the
>  .Cm join
>  list will record
>  .Cm wpakey ,
> +.Cm wpakeyfile ,
>  .Cm wpaprotos ,
>  or
>  .Cm nwkey
> @@ -1209,6 +1211,8 @@ The default value is
>  .Dq psk
>  can only be used if a pre-shared key is configured using the
>  .Cm wpakey
> +or
> +.Cm wpakeyfile
>  option.
>  .It Cm wpaciphers Ar cipher,cipher,...
>  Set the comma-separated list of allowed pairwise ciphers.
> @@ -1268,6 +1272,10 @@ or
>  option must first be specified, since
>  .Nm
>  will hash the nwid along with the passphrase to create the key.
> +.It Cm wpakeyfile Ar path
> +Set the WPA key contained in the file at the specified
> +.Ar path .
> +Trailing whitespace is ignored.
>  .It Cm -wpakey
>  Delete the pre-shared WPA key and disable WPA.
>  .It Cm wpaprotos Ar proto,proto,...
> blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b
> file + sbin/ifconfig/ifconfig.c
> --- sbin/ifconfig/ifconfig.c
> +++ sbin/ifconfig/ifconfig.c
> @@ -63,6 +63,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -106,6 +107,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #ifndef SMALL
>  #include 
> @@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int);
>  void   setifwpaciphers(const char *, int);
>  void   setifwpagroupcipher(const char *, int);
>  void   setifwpakey(const char *, int);
> +void   setifwpakeyfile(const char *, int);
>  void   setifchan(const char *, int);
>  void   setifscan(const char *, int);
>  void   setifnwflag(const char *, int);
> @@ -415,6 +418,7 @@ const structcmd {
> { "wpagroupcipher", NEXTARG,0,
> setifwpagroupcipher },
> { "wpaprotos",  NEXTARG,0,  setifwpaprotos },
> { "wpakey", NEXTARG,0,  setifwpakey },
> +   { "wpakeyfile", NEXTARG,0,  setifwpakeyfile },
> { "-wpakey",-1, 0,  setifwpakey },
> { "chan",   NEXTARG0,   0,  setifchan },
> { "-chan",  -1, 0,  setifchan },
> @@ -728,7 +732,7 @@ main(int argc, char *argv[])
> int create = 0;
> int Cflag = 0;
> int gflag = 0;
> -   int found_rulefile = 0;
> +   int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0;
> int i;
>
> /* If no args at all, print all interfaces.  */
> @@ -785,9 +789,13 @@ main(int argc, char *argv[])
> found_rulefile = 1;
> break;
> }
> +   if (strcmp(argv[i], "wpakeyfile") == 0) {
> +   found_wpakeyfile = 1;
> +   break;
> +   }
> }
>
> -   if (!found_rulefile) {
> +   if (!found_rulefile && !found_wpakeyfile) {
> if (unveil(_PATH_RESCONF, "r") == -1)
> err(1, "unveil");
> if (unveil(_PATH_HOSTS, "r") == -1)
> @@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d)
> wpa.i_enabled = psk.i_enabled;
> if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1)
> err(1, "SIOCS80211WPAPARMS");
> +}
> +
> +void
> +setifwpakeyfile(const char *val, int d)
> +{
> +   char *wpakey;
> +   int fd;
> +   struct stat sb;
> +   ssize_t n;
> +
> +   fd = open(val, O_RDONLY);
> +   if (fd == -1)
> +   err(1, "open %s", val);
> +
> +   if (fstat(fd, ) == -1)
> +   err(1, "fstat %s", val);
> +
> +   wpakey = malloc(sb.st_size);
> +   if (wpakey == NULL)
> +   err(1, "malloc");
> +
> +   n = read(fd, wpakey, sb.st_size);
> +   if (n == -1)
> +   err(1, "read %s", val);
> +   if (n != sb.st_size)
> +   errx(1, "failed to read from file %s", val);
> +   close(fd);
> +

Re: umb(4) WIP diff and questions

2020-01-14 Thread Stefan Sperling
On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote:
> Channeling a conversation from 15 years ago: "How about wpakeyfile"

ifconfig wpakeyfile would be trivial to add if we really want it.

The downside is loss of unveil, here handled the same way as for the
bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad
practice even for an 'i' that should contain a path?

diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src
blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d
file + sbin/ifconfig/ifconfig.8
--- sbin/ifconfig/ifconfig.8
+++ sbin/ifconfig/ifconfig.8
@@ -940,6 +940,7 @@ will begin advertising as master.
 .Op Cm wpaciphers Ar cipher,cipher,...
 .Op Cm wpagroupcipher Ar cipher
 .Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey
+.Op Cm wpakeyfile Ar path
 .Op Cm wpaprotos Ar proto,proto,...
 .Ek
 .nr nS 0
@@ -990,6 +991,7 @@ the
 .Cm join
 list will record
 .Cm wpakey ,
+.Cm wpakeyfile ,
 .Cm wpaprotos ,
 or
 .Cm nwkey
@@ -1209,6 +1211,8 @@ The default value is
 .Dq psk
 can only be used if a pre-shared key is configured using the
 .Cm wpakey
+or
+.Cm wpakeyfile
 option.
 .It Cm wpaciphers Ar cipher,cipher,...
 Set the comma-separated list of allowed pairwise ciphers.
@@ -1268,6 +1272,10 @@ or
 option must first be specified, since
 .Nm
 will hash the nwid along with the passphrase to create the key.
+.It Cm wpakeyfile Ar path
+Set the WPA key contained in the file at the specified
+.Ar path .
+Trailing whitespace is ignored.
 .It Cm -wpakey
 Delete the pre-shared WPA key and disable WPA.
 .It Cm wpaprotos Ar proto,proto,...
blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b
file + sbin/ifconfig/ifconfig.c
--- sbin/ifconfig/ifconfig.c
+++ sbin/ifconfig/ifconfig.c
@@ -63,6 +63,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -106,6 +107,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifndef SMALL
 #include 
@@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int);
 void   setifwpaciphers(const char *, int);
 void   setifwpagroupcipher(const char *, int);
 void   setifwpakey(const char *, int);
+void   setifwpakeyfile(const char *, int);
 void   setifchan(const char *, int);
 void   setifscan(const char *, int);
 void   setifnwflag(const char *, int);
@@ -415,6 +418,7 @@ const structcmd {
{ "wpagroupcipher", NEXTARG,0,  setifwpagroupcipher },
{ "wpaprotos",  NEXTARG,0,  setifwpaprotos },
{ "wpakey", NEXTARG,0,  setifwpakey },
+   { "wpakeyfile", NEXTARG,0,  setifwpakeyfile },
{ "-wpakey",-1, 0,  setifwpakey },
{ "chan",   NEXTARG0,   0,  setifchan },
{ "-chan",  -1, 0,  setifchan },
@@ -728,7 +732,7 @@ main(int argc, char *argv[])
int create = 0;
int Cflag = 0;
int gflag = 0;
-   int found_rulefile = 0;
+   int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0;
int i;
 
/* If no args at all, print all interfaces.  */
@@ -785,9 +789,13 @@ main(int argc, char *argv[])
found_rulefile = 1;
break;
}
+   if (strcmp(argv[i], "wpakeyfile") == 0) {
+   found_wpakeyfile = 1;
+   break;
+   }
}
 
-   if (!found_rulefile) {
+   if (!found_rulefile && !found_wpakeyfile) {
if (unveil(_PATH_RESCONF, "r") == -1)
err(1, "unveil");
if (unveil(_PATH_HOSTS, "r") == -1)
@@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d)
wpa.i_enabled = psk.i_enabled;
if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1)
err(1, "SIOCS80211WPAPARMS");
+}
+
+void
+setifwpakeyfile(const char *val, int d)
+{
+   char *wpakey;
+   int fd;
+   struct stat sb;
+   ssize_t n;
+
+   fd = open(val, O_RDONLY);
+   if (fd == -1)
+   err(1, "open %s", val);
+
+   if (fstat(fd, ) == -1)
+   err(1, "fstat %s", val);
+
+   wpakey = malloc(sb.st_size);
+   if (wpakey == NULL)
+   err(1, "malloc");
+   
+   n = read(fd, wpakey, sb.st_size);
+   if (n == -1)
+   err(1, "read %s", val);
+   if (n != sb.st_size)
+   errx(1, "failed to read from file %s", val);
+   close(fd);
+
+   while (n > 0 && isspace(wpakey[n - 1])) {
+   wpakey[n - 1] = '\0';
+   n--;
+   }
+
+   setifwpakey(wpakey, d);
 }
 
 void




Re: umb(4) WIP diff and questions

2020-01-14 Thread Theo de Raadt
Theo de Raadt  wrote:

> Stuart Henderson  wrote:
> 
> > On 2020/01/14 10:27, Theo de Raadt wrote:
> > > Unfortunate part of this diff is that the password is (very
> > > momentarily) visible with ps(1) in the root-run ifconfig argv[] array.
> > > It's a tight race, but still it is visible.
> > > 
> > > People do run "sh /etc/netstart umb0" to activate the interface
> > > during multiuser.
> > > 
> > > If the password is truly sensitive, it should be placed in a file,
> > > and the ifconfig's extension should be changed to read the file.
> > 
> > That's not unique to umb though, it's been a problem forever with carp,
> > pppoe and especially wlan interfaces.
> 
> When creating new versions of the same problem... that's a great time
> to reconsider the old solutions.
> 
> > Another fix would be to accept
> > ifconfig options on stdin, which is more convenient for quick runtime
> > changes than two steps of writing to a file then pointing ifconfig at
> > it, and changing netstart to use it would improve things for existing
> > users without them needing to touch any config files.
> 
> I think using the shell is silly, because what if there are two such
> options?  Then you can't seperate the stdin.
> 
> Additionally, most of the time when creating such temporary configuration,
> the pattern is that if it works you'll want to apply it permanent.
> 
> Another thought is to create both versions of the option.  One on the
> commandline, and one pointing at a file.
> 
> Channeling a conversation from 15 years ago: "How about wpakeyfile"


Another consideration is... many of these passwords are locked to narrow
usage cases, so does it really matter all that much?



Re: umb(4) WIP diff and questions

2020-01-14 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2020/01/14 10:27, Theo de Raadt wrote:
> > Unfortunate part of this diff is that the password is (very
> > momentarily) visible with ps(1) in the root-run ifconfig argv[] array.
> > It's a tight race, but still it is visible.
> > 
> > People do run "sh /etc/netstart umb0" to activate the interface
> > during multiuser.
> > 
> > If the password is truly sensitive, it should be placed in a file,
> > and the ifconfig's extension should be changed to read the file.
> 
> That's not unique to umb though, it's been a problem forever with carp,
> pppoe and especially wlan interfaces.

When creating new versions of the same problem... that's a great time
to reconsider the old solutions.

> Another fix would be to accept
> ifconfig options on stdin, which is more convenient for quick runtime
> changes than two steps of writing to a file then pointing ifconfig at
> it, and changing netstart to use it would improve things for existing
> users without them needing to touch any config files.

I think using the shell is silly, because what if there are two such
options?  Then you can't seperate the stdin.

Additionally, most of the time when creating such temporary configuration,
the pattern is that if it works you'll want to apply it permanent.

Another thought is to create both versions of the option.  One on the
commandline, and one pointing at a file.

Channeling a conversation from 15 years ago: "How about wpakeyfile"



Re: umb(4) WIP diff and questions

2020-01-14 Thread Stuart Henderson
On 2020/01/14 10:27, Theo de Raadt wrote:
> Unfortunate part of this diff is that the password is (very
> momentarily) visible with ps(1) in the root-run ifconfig argv[] array.
> It's a tight race, but still it is visible.
> 
> People do run "sh /etc/netstart umb0" to activate the interface
> during multiuser.
> 
> If the password is truly sensitive, it should be placed in a file,
> and the ifconfig's extension should be changed to read the file.

That's not unique to umb though, it's been a problem forever with carp,
pppoe and especially wlan interfaces. Another fix would be to accept
ifconfig options on stdin, which is more convenient for quick runtime
changes than two steps of writing to a file then pointing ifconfig at
it, and changing netstart to use it would improve things for existing
users without them needing to touch any config files.



Re: umb(4) WIP diff and questions

2020-01-14 Thread Theo de Raadt
Unfortunate part of this diff is that the password is (very
momentarily) visible with ps(1) in the root-run ifconfig argv[] array.
It's a tight race, but still it is visible.

People do run "sh /etc/netstart umb0" to activate the interface
during multiuser.

If the password is truly sensitive, it should be placed in a file,
and the ifconfig's extension should be changed to read the file.



Re: umb(4) WIP diff and questions

2020-01-14 Thread Claudio Jeker
On Tue, Jan 14, 2020 at 02:40:45PM +0100, Stefan Sperling wrote:
> On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote:
> > Hello again tech@
> > 
> > I've included diffs of what I've got so far at the bottom 
> > of this mail, but first a couple of questions:
> > 
> > - Using the full 510-character limits for username and
> > passphrase specified in the MBIM spec, kernel compilation 
> > fails due to tripping the 2047-byte stack frame warning
> > when compiling the driver. That's the reason for the '100'
> > magic numbers that I put in there temporarily.
> > 
> > Any hints on the best way to handle this would be appreciated. 
> 
> I would allocate mp dynamically instead of storing it on the stack.
> 
> In umb_ioctl(), you could change mp to a pointer type:
> 
>   struct umb_parameter *mp;
> 
> The ioctl is allowed to sleep until memory is available (M_WAITOK), so
> this allocation cannot fail and you don't need to check for NULL:
> 
>   mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK);
> 
> free mp before umb_ioctl returns:
> 
>   free(mp, M_DEVBUF, sizeof(*mp));
> 
> See 'man 9 malloc' for details.
> 
> > - I included the username/passphrase fields in the ifconfig
> > output, as it seems to me that APN settings are generally
> > made public so end-users can configure their own devices
> > If needed I'll take it out, or perhaps change it to display 
> > '*set*' (or similar) if you think it should be hidden?
> 
> Genrally, the kernel should not return credentials to userland.
> So these shouldn't just be hidden or obscured in ifconfig. Rather,
> umb_ioctl should not copy such data out to any userland program.
> 
> This rule also applies to wifi and carp interfaces, for instance.
>  
> > A couple of other points:
> > 
> > - Wireless providers where I am all seem to require a
> > username/password with the APN. So I'm unable to confirm
> > whether or not this breaks non-authenticated connections.
> > 
> > - I've been building my kernel using the documented 
> > procedure, and have no problems there. I ran through a
> > base system build and that (eventually) completed OK too.
> > When compiling ifconfig(8), I've been doing 'make includes'
> > in /usr/src (after installing and booting my new kernel), 
> > and using 'make ifconfig' and 'make install' in 
> > /usr/src/sbin/ifconfig. Is this OK? or should I be doing 
> > something else instead?
> 
> You should verify that 'make release' works after having completed a full
> system build. The ramdisks use a special build of ifconfig so a check that
> this still compiles is required. See 'man release' for details.
> 

Since the credentials should not be passed back to userland I would not
add them to struct umb_parameter but instead to struct umb_softc.
Then you don't need to use struct umb_parameter for the ioctl and instead
could just pass the (utf16) string to the kernel.
Apart form that it looks good.

-- 
:wq Claudio



Re: umb(4) WIP diff and questions

2020-01-14 Thread Stefan Sperling
On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote:
> Hello again tech@
> 
> I've included diffs of what I've got so far at the bottom 
> of this mail, but first a couple of questions:
> 
> - Using the full 510-character limits for username and
> passphrase specified in the MBIM spec, kernel compilation 
> fails due to tripping the 2047-byte stack frame warning
> when compiling the driver. That's the reason for the '100'
> magic numbers that I put in there temporarily.
> 
> Any hints on the best way to handle this would be appreciated. 

I would allocate mp dynamically instead of storing it on the stack.

In umb_ioctl(), you could change mp to a pointer type:

struct umb_parameter *mp;

The ioctl is allowed to sleep until memory is available (M_WAITOK), so
this allocation cannot fail and you don't need to check for NULL:

mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK);

free mp before umb_ioctl returns:

free(mp, M_DEVBUF, sizeof(*mp));

See 'man 9 malloc' for details.

> - I included the username/passphrase fields in the ifconfig
> output, as it seems to me that APN settings are generally
> made public so end-users can configure their own devices
> If needed I'll take it out, or perhaps change it to display 
> '*set*' (or similar) if you think it should be hidden?

Genrally, the kernel should not return credentials to userland.
So these shouldn't just be hidden or obscured in ifconfig. Rather,
umb_ioctl should not copy such data out to any userland program.

This rule also applies to wifi and carp interfaces, for instance.
 
> A couple of other points:
> 
> - Wireless providers where I am all seem to require a
> username/password with the APN. So I'm unable to confirm
> whether or not this breaks non-authenticated connections.
> 
> - I've been building my kernel using the documented 
> procedure, and have no problems there. I ran through a
> base system build and that (eventually) completed OK too.
> When compiling ifconfig(8), I've been doing 'make includes'
> in /usr/src (after installing and booting my new kernel), 
> and using 'make ifconfig' and 'make install' in 
> /usr/src/sbin/ifconfig. Is this OK? or should I be doing 
> something else instead?

You should verify that 'make release' works after having completed a full
system build. The ramdisks use a special build of ifconfig so a check that
this still compiles is required. See 'man release' for details.