brconfig: strto*l -> strtonum()

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

brconfig: strto*l -> strtonum()

Klemens Nanni-2

Poking and testing around in brconfig.c for tpmr(4) stuff, I noticed a
lot of old code around strto*l(3).

Many pass unbounded `long' values into the `[u]int32_t' struct members
without limiting them to at least the type size the value is stored in,
some report wrong commands in error messages, e.g.

        # ifconfig switch1 portno vether1 9223372036854775808 # LONG_MAX + 1
        ifconfig: invalid arg for portidx: 9223372036854775808

some pass values to the kernel that are above limits documented in
ifconfig(8), e.g.

        #  ifconfig bridge0 maxage 50
        ifconfig: bridge0: Invalid argument

and others overflow in-kernel due to that (causing persistent damage
that causes deleting and adding bridge members to fix it).

I'm aware that moving to strtonum(3) drops the ability to pass numbers
in bases others than ten, e.g. `ifconfig bridge0 ifcost vether0 0x3'
mut now be `... 15', but all of the options really want a decimal number
as far as I can judge after reading ifconfig(8) and testing them.

The only exception to this is switch(4)'s `datapath' which is printed in
hexadecimal, so I left it untouched such that it can be copy-pasted and
set as is.

Diff below applies converts all places to the very same strtonum() idiom
with proper range checks based on documented or type limits.

I've tested each option manually: valid values are still valid and but
more out of range values are catched now, many of them up front in
ifconfig rather than the kerne's ioctl() path.

I've also checked most of the ioctl() paths to see what those really
check for.  One or two diffs for the kernel should follow soon.

Feedback? OK?


Index: brconfig.c
===================================================================
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.26
diff -u -p -r1.26 brconfig.c
--- brconfig.c 29 Jul 2020 12:13:28 -0000 1.26
+++ brconfig.c 29 Jul 2020 17:06:09 -0000
@@ -418,18 +418,13 @@ void
 bridge_timeout(const char *arg, int d)
 {
  struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- newtime = strtol(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
-    (newtime & ~INT_MAX) != 0L ||
-    (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, UINT32_MAX, &errstr);
+ if (errstr)
+ err(1, "timeout %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
  if (ioctl(sock, SIOCBRDGSTO, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -438,17 +433,13 @@ void
 bridge_maxage(const char *arg, int d)
 {
  struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 6, 40, &errstr);
+ if (errstr)
+ errx(1, "maxage %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
  if (ioctl(sock, SIOCBRDGSMA, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -457,17 +448,13 @@ void
 bridge_priority(const char *arg, int d)
 {
  struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio  = strtonum(arg, 0, 61440, &errstr);
+ if (errstr)
+ errx(1, "spanpriority %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
  if (ioctl(sock, SIOCBRDGSPRI, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -478,7 +465,7 @@ bridge_protect(const char *ifsname, cons
  struct ifbreq breq;
  unsigned long v;
  char *optlist, *str;
- char *endptr;
+ const char *errstr;
 
  strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
  strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
@@ -491,11 +478,9 @@ bridge_protect(const char *ifsname, cons
 
  str = strtok(optlist, ",");
  while (str != NULL) {
- errno = 0;
- v = strtoul(str, &endptr, 0);
- if (str[0] == '\0' || endptr[0] != '\0' || v == 0 || v > 31 ||
-    (errno == ERANGE && v == ULONG_MAX))
- err(1, "invalid value for protected domain: %s", str);
+ v = strtonum(str, 1, 31, &errstr);
+ if (errstr)
+ err(1, "protected domain %s is: %s", str, errstr);
  breq.ifbr_protected |= (1 << (v - 1));
  str = strtok(NULL, ",");
  }
@@ -544,17 +529,14 @@ void
 bridge_fwddelay(const char *arg, int d)
 {
  struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ bp.ifbrp_fwddelay = strtonum(arg, 4, 30, &errstr);
+ if (errstr)
+ errx(1, "fwddelay %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_fwddelay = v;
+
  if (ioctl(sock, SIOCBRDGSFD, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -563,17 +545,14 @@ void
 bridge_hellotime(const char *arg, int d)
 {
  struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- v = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for hellotime: %s", arg);
+ bp.ifbrp_hellotime = strtonum(arg, 1, 2, &errstr);
+ if (errstr)
+ errx(1, "hellotime %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_hellotime = v;
+
  if (ioctl(sock, SIOCBRDGSHT, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -583,16 +562,13 @@ bridge_maxaddr(const char *arg, int d)
 {
  struct ifbrparam bp;
  unsigned long newsize;
- char *endptr;
+ const char *errstr;
 
- errno = 0;
- newsize = strtoul(arg, &endptr, 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || newsize > 0xffffffffUL ||
-    (errno == ERANGE && newsize == ULONG_MAX))
- errx(1, "invalid arg for maxaddr: %s", arg);
+ bp.ifbrp_csize = strtonum(arg, 0, UINT32_MAX, &errstr);
+ if (errstr)
+ errx(1, "maxaddr %s is: %s", arg, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
- bp.ifbrp_csize = newsize;
  if (ioctl(sock, SIOCBRDGSCACHE, (caddr_t)&bp) == -1)
  err(1, "%s", ifname);
 }
@@ -618,19 +594,15 @@ void
 bridge_ifprio(const char *ifsname, const char *val)
 {
  struct ifbreq breq;
- unsigned long v;
- char *endptr;
+ const char *errstr;
+
+ breq.ifbr_priority = strtonum(val, 0, 240, &errstr);
+ if (errstr)
+ errx(1, "ifpriority %s is: %s", val, errstr);
 
  strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
  strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
 
- errno = 0;
- v = strtoul(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- err(1, "invalid arg for ifpriority: %s", val);
- breq.ifbr_priority = v;
-
  if (ioctl(sock, SIOCBRDGSIFPRIO, (caddr_t)&breq) == -1)
  err(1, "%s: %s", ifname, val);
 }
@@ -639,20 +611,15 @@ void
 bridge_ifcost(const char *ifsname, const char *val)
 {
  struct ifbreq breq;
- unsigned long v;
- char *endptr;
+ const char *errstr;
+
+ breq.ifbr_path_cost = strtonum(val, 1, 200000000, &errstr);
+ if (errstr)
+ errx(1, "ifcost %s is: %s", val, errstr);
 
  strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
  strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
 
- errno = 0;
- v = strtoul(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffffffffUL ||
-    (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for ifcost: %s", val);
-
- breq.ifbr_path_cost = v;
-
  if (ioctl(sock, SIOCBRDGSIFCOST, (caddr_t)&breq) == -1)
  err(1, "%s: %s", ifname, val);
 }
@@ -747,9 +714,9 @@ bridge_holdcnt(const char *value, int d)
  struct ifbrparam bp;
  const char *errstr;
 
- bp.ifbrp_txhc = strtonum(value, 0, UINT8_MAX, &errstr);
+ bp.ifbrp_txhc = strtonum(value, 1, 11, &errstr);
  if (errstr)
- err(1, "holdcnt %s %s", value, errstr);
+ err(1, "holdcnt %s is: %s", value, errstr);
 
  strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
  if (ioctl(sock, SIOCBRDGSTXHC, (caddr_t)&bp) == -1)
@@ -1206,18 +1173,14 @@ void
 switch_portno(const char *ifsname, const char *val)
 {
  struct ifbreq breq;
- uint32_t newportidx;
- char *endptr;
+ const char *errstr;
+
+ breq.ifbr_portno = strtonum(val, 0, UINT32_MAX, &errstr);
+ if (errstr)
+ errx(1, "portno %s is: %s", val, errstr);
 
  strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
  strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
-
- errno = 0;
- newportidx = strtol(val, &endptr, 0);
- if (val[0] == '\0' || endptr[0] != '\0' || errno == ERANGE)
- errx(1, "invalid arg for portidx: %s", val);
-
- breq.ifbr_portno = newportidx;
  if (ioctl(sock, SIOCSWSPORTNO, (caddr_t)&breq) == -1) {
  if (errno == EEXIST)
  return;