On Wed, 27 Sep 2017 18:05:28 +0200
Phil Sutter <p...@nwl.cc> wrote:
> On Wed, Sep 27, 2017 at 08:42:49AM +0100, Stephen Hemminger wrote:
> > On Tue, 26 Sep 2017 18:35:45 +0200
> > Phil Sutter <p...@nwl.cc> wrote:
> >
> > > This series adds explicit checks for user-supplied interface names to
> > > make sure their length fits Linux's requirements.
> > >
> > > The first two patches simplify interface name parsing in some places -
> > > these are side-effects of working on the actual implementation provided
> > > in patch three.
> > >
> > > Changes since v1:
> > > - Patches 1 and 2 introduced.
> > > - Changes to patch 3 are listed in there.
> > >
> > > Phil Sutter (3):
> > > ip{6,}tunnel: Avoid copying user-supplied interface name around
> > > tc: flower: No need to cache indev arg
> > > Check user supplied interface name lengths
> > >
> > > include/utils.h | 1 +
> > > ip/ip6tunnel.c | 9 +++++----
> > > ip/ipl2tp.c | 3 ++-
> > > ip/iplink.c | 27 ++++++++-------------------
> > > ip/ipmaddr.c | 1 +
> > > ip/iprule.c | 4 ++++
> > > ip/iptunnel.c | 27 +++++++++++++--------------
> > > ip/iptuntap.c | 4 +++-
> > > lib/utils.c | 10 ++++++++++
> > > misc/arpd.c | 1 +
> > > tc/f_flower.c | 6 ++----
> > > 11 files changed, 50 insertions(+), 43 deletions(-)
> > >
> >
> > I like the idea, and checking arguments is good.
>
> Cool!
I was thinking something like:
diff --git a/include/utils.h b/include/utils.h
index c9ed230b9604..e2702b56f2e0 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -105,6 +105,8 @@ int get_be64(__be64 *val, const char *arg, int base);
int get_be32(__be32 *val, const char *arg, int base);
int get_be16(__be16 *val, const char *arg, int base);
int get_addr64(__u64 *ap, const char *cp);
+int check_ifname(const char *arg);
+int get_ifname(char *buf, const char *arg);
int hex2mem(const char *buf, uint8_t *mem, int count);
char *hexstring_n2a(const __u8 *str, int len, char *buf, int blen);
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def14422..a6f0e99bdc21 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -180,7 +180,8 @@ static int parse_args(int argc, char **argv, int cmd,
struct ip6_tnl_parm2 *p)
memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
- strncpy(medium, *argv, IFNAMSIZ - 1);
+ if (get_ifname(medium, *argv))
+ invarg("\"medium\" not a valid ifname", *argv);
} else if (strcmp(*argv, "encaplimit") == 0) {
NEXT_ARG();
if (strcmp(*argv, "none") == 0) {
diff --git a/ip/iplink.c b/ip/iplink.c
index ff5b56c038d2..89aa51ed3b40 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1265,6 +1265,8 @@ static int do_set(int argc, char **argv)
flags &= ~IFF_UP;
} else if (strcmp(*argv, "name") == 0) {
NEXT_ARG();
+ if (check_ifname(*argv))
+ invarg("Invalid \"name\"\n", *argv);
newname = *argv;
} else if (matches(*argv, "address") == 0) {
NEXT_ARG();
@@ -1383,9 +1385,6 @@ static int do_set(int argc, char **argv)
}
if (newname && strcmp(dev, newname)) {
- if (strlen(newname) == 0)
- invarg("\"\" is not a valid device identifier\n",
- "name");
if (do_changename(dev, newname) < 0)
return -1;
dev = newname;
diff --git a/lib/utils.c b/lib/utils.c
index bbd3cbc46a0e..a93b45b51a3b 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -20,6 +20,7 @@
#include <sys/socket.h>
#include <netinet/in.h>
#include <string.h>
+#include <ctype.h>
#include <netdb.h>
#include <arpa/inet.h>
#include <asm/types.h>
@@ -465,6 +466,34 @@ int get_addr64(__u64 *ap, const char *cp)
return 1;
}
+int check_ifname(const char *name)
+{
+ /* These check mimic kernel checks in dev_valid_name */
+ if (*name == '\0')
+ return -1;
+ if (strlen(name) >= IFNAMSIZ)
+ return -1;
+
+ while (*name) {
+ if (*name == '/' || isspace(*name))
+ return -1;
+ ++name;
+ }
+ return 0;
+}
+
+/* buf is assumed to be IFNAMSIZ */
+int get_ifname(char *buf, const char *name)
+{
+ int ret;
+
+ ret = check_ifname(name);
+ if (ret == 0)
+ strncpy(buf, name, IFNAMSIZ - 1);
+
+ return ret;
+}
+
int get_addr_1(inet_prefix *addr, const char *name, int family)
{
memset(addr, 0, sizeof(*addr));
--
2.11.0
