Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/818734/?format=api
{ "id": 818734, "url": "http://patchwork.ozlabs.org/api/patches/818734/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/20170926163548.24347-4-phil@nwl.cc/", "project": { "id": 7, "url": "http://patchwork.ozlabs.org/api/projects/7/?format=api", "name": "Linux network development", "link_name": "netdev", "list_id": "netdev.vger.kernel.org", "list_email": "netdev@vger.kernel.org", "web_url": null, "scm_url": null, "webscm_url": null, "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170926163548.24347-4-phil@nwl.cc>", "list_archive_url": null, "date": "2017-09-26T16:35:48", "name": "[iproute,v2,3/3] Check user supplied interface name lengths", "commit_ref": null, "pull_url": null, "state": "changes-requested", "archived": true, "hash": "5e1bb66d4435497780935c462d5be86ee41c9c47", "submitter": { "id": 4285, "url": "http://patchwork.ozlabs.org/api/people/4285/?format=api", "name": "Phil Sutter", "email": "phil@nwl.cc" }, "delegate": { "id": 389, "url": "http://patchwork.ozlabs.org/api/users/389/?format=api", "username": "shemminger", "first_name": "stephen", "last_name": "hemminger", "email": "shemminger@vyatta.com" }, "mbox": "http://patchwork.ozlabs.org/project/netdev/patch/20170926163548.24347-4-phil@nwl.cc/mbox/", "series": [ { "id": 5182, "url": "http://patchwork.ozlabs.org/api/series/5182/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/list/?series=5182", "date": "2017-09-26T16:35:46", "name": "Check user supplied interface name lengths", "version": 2, "mbox": "http://patchwork.ozlabs.org/series/5182/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/818734/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/818734/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<netdev-owner@vger.kernel.org>", "X-Original-To": "patchwork-incoming@ozlabs.org", "Delivered-To": "patchwork-incoming@ozlabs.org", "Authentication-Results": "ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)", "Received": [ "from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1mmb4Gzjz9t67\n\tfor <patchwork-incoming@ozlabs.org>;\n\tWed, 27 Sep 2017 02:36:15 +1000 (AEST)", "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S968965AbdIZQgN (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tTue, 26 Sep 2017 12:36:13 -0400", "from orbyte.nwl.cc ([151.80.46.58]:58112 \"EHLO orbyte.nwl.cc\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S967740AbdIZQgL (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tTue, 26 Sep 2017 12:36:11 -0400", "from localhost ([::1]:52654 helo=xsao)\n\tby orbyte.nwl.cc with esmtp (Exim 4.89)\n\t(envelope-from <phil@nwl.cc>)\n\tid 1dwsqA-0008HI-BI; Tue, 26 Sep 2017 18:36:10 +0200" ], "From": "Phil Sutter <phil@nwl.cc>", "To": "Stephen Hemminger <stephen@networkplumber.org>", "Cc": "netdev@vger.kernel.org", "Subject": "[iproute PATCH v2 3/3] Check user supplied interface name lengths", "Date": "Tue, 26 Sep 2017 18:35:48 +0200", "Message-Id": "<20170926163548.24347-4-phil@nwl.cc>", "X-Mailer": "git-send-email 2.13.1", "In-Reply-To": "<20170926163548.24347-1-phil@nwl.cc>", "References": "<20170926163548.24347-1-phil@nwl.cc>", "Sender": "netdev-owner@vger.kernel.org", "Precedence": "bulk", "List-ID": "<netdev.vger.kernel.org>", "X-Mailing-List": "netdev@vger.kernel.org" }, "content": "The original problem was that something like:\n\n| strncpy(ifr.ifr_name, *argv, IFNAMSIZ);\n\nmight leave ifr.ifr_name unterminated if length of *argv exceeds\nIFNAMSIZ. In order to fix this, I thought about replacing all those\ncases with (equivalent) calls to snprintf() or even introducing\nstrlcpy(). But as Ulrich Drepper correctly pointed out when rejecting\nthe latter from being added to glibc, truncating a string without\nnotifying the user is not to be considered good practice. So let's\nexcercise what he suggested and reject empty or overlong interface names\nright from the start - this way calls to strncpy() like shown above\nbecome safe and the user has a chance to reconsider what he was trying\nto do.\n\nNote that this doesn't add calls to check_ifname() to all places where\nuser supplied interface name is parsed. In many cases, the interface\nmust exist already and is therefore looked up using ll_name_to_index(),\nso if_nametoindex() will perform the necessary checks already.\n\nSigned-off-by: Phil Sutter <phil@nwl.cc>\n---\nChanges since v1:\n- added missing check to tc/f_flower.c\n- Drop some useless checks from ip/ip{6,}tunnel.c (ll_name_to_index()\n will detect illegal interface names for us).\n- Renamed assert_valid_dev_name() to the shorter check_ifname().\n- iplink: Check 'name' and 'dev' parameters right where they are parsed.\n- ipl2tp: Drop needless check for p->ifname[0].\n---\n include/utils.h | 1 +\n ip/ip6tunnel.c | 3 ++-\n ip/ipl2tp.c | 3 ++-\n ip/iplink.c | 27 ++++++++-------------------\n ip/ipmaddr.c | 1 +\n ip/iprule.c | 4 ++++\n ip/iptunnel.c | 5 ++++-\n ip/iptuntap.c | 4 +++-\n lib/utils.c | 10 ++++++++++\n misc/arpd.c | 1 +\n tc/f_flower.c | 1 +\n 11 files changed, 37 insertions(+), 23 deletions(-)", "diff": "diff --git a/include/utils.h b/include/utils.h\nindex c9ed230b96044..12f0735e8aa0d 100644\n--- a/include/utils.h\n+++ b/include/utils.h\n@@ -133,6 +133,7 @@ void missarg(const char *) __attribute__((noreturn));\n void invarg(const char *, const char *) __attribute__((noreturn));\n void duparg(const char *, const char *) __attribute__((noreturn));\n void duparg2(const char *, const char *) __attribute__((noreturn));\n+void check_ifname(const char *, const char *);\n int matches(const char *arg, const char *pattern);\n int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);\n \ndiff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c\nindex c12d700e74189..2c10b9c3fa7b5 100644\n--- a/ip/ip6tunnel.c\n+++ b/ip/ip6tunnel.c\n@@ -273,7 +273,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)\n \t\t\t\tusage();\n \t\t\tif (p->name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n-\t\t\tstrncpy(p->name, *argv, IFNAMSIZ - 1);\n+\t\t\tcheck_ifname(\"name\", *argv);\n+\t\t\tstrncpy(p->name, *argv, IFNAMSIZ);\n \t\t\tif (cmd == SIOCCHGTUNNEL && count == 0) {\n \t\t\t\tstruct ip6_tnl_parm2 old_p = {};\n \ndiff --git a/ip/ipl2tp.c b/ip/ipl2tp.c\nindex 88664c909e11f..06f1bd064c914 100644\n--- a/ip/ipl2tp.c\n+++ b/ip/ipl2tp.c\n@@ -182,7 +182,7 @@ static int create_session(struct l2tp_parm *p)\n \tif (p->peer_cookie_len)\n \t\taddattr_l(&req.n, 1024, L2TP_ATTR_PEER_COOKIE,\n \t\t\t p->peer_cookie, p->peer_cookie_len);\n-\tif (p->ifname && p->ifname[0])\n+\tif (p->ifname)\n \t\taddattrstrz(&req.n, 1024, L2TP_ATTR_IFNAME, p->ifname);\n \n \tif (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)\n@@ -545,6 +545,7 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)\n \t\t\t}\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\tp->ifname = *argv;\n \t\t} else if (strcmp(*argv, \"remote\") == 0) {\n \t\t\tNEXT_ARG();\ndiff --git a/ip/iplink.c b/ip/iplink.c\nindex ff5b56c038d28..f000a7992c92e 100644\n--- a/ip/iplink.c\n+++ b/ip/iplink.c\n@@ -573,6 +573,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \t\t\treq->i.ifi_flags &= ~IFF_UP;\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\t*name = *argv;\n \t\t} else if (strcmp(*argv, \"index\") == 0) {\n \t\t\tNEXT_ARG();\n@@ -848,6 +849,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \t\t\t\tNEXT_ARG();\n \t\t\tif (*dev)\n \t\t\t\tduparg2(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\t*dev = *argv;\n \t\t\tdev_index = ll_name_to_index(*dev);\n \t\t}\n@@ -870,7 +872,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \n static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n {\n-\tint len;\n \tchar *dev = NULL;\n \tchar *name = NULL;\n \tchar *link = NULL;\n@@ -960,13 +961,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n \t}\n \n \tif (name) {\n-\t\tlen = strlen(name) + 1;\n-\t\tif (len == 1)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t \"name\");\n-\t\tif (len > IFNAMSIZ)\n-\t\t\tinvarg(\"\\\"name\\\" too long\\n\", name);\n-\t\taddattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);\n+\t\taddattr_l(&req.n, sizeof(req),\n+\t\t\t IFLA_IFNAME, name, strlen(name) + 1);\n \t}\n \n \tif (type) {\n@@ -1016,7 +1012,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n \n int iplink_get(unsigned int flags, char *name, __u32 filt_mask)\n {\n-\tint len;\n \tstruct iplink_req req = {\n \t\t.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),\n \t\t.n.nlmsg_flags = NLM_F_REQUEST | flags,\n@@ -1029,13 +1024,8 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)\n \t} answer;\n \n \tif (name) {\n-\t\tlen = strlen(name) + 1;\n-\t\tif (len == 1)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t\t \"name\");\n-\t\tif (len > IFNAMSIZ)\n-\t\t\tinvarg(\"\\\"name\\\" too long\\n\", name);\n-\t\taddattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);\n+\t\taddattr_l(&req.n, sizeof(req),\n+\t\t\t IFLA_IFNAME, name, strlen(name) + 1);\n \t}\n \taddattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);\n \n@@ -1265,6 +1255,7 @@ static int do_set(int argc, char **argv)\n \t\t\tflags &= ~IFF_UP;\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", newname);\n \t\t\tnewname = *argv;\n \t\t} else if (matches(*argv, \"address\") == 0) {\n \t\t\tNEXT_ARG();\n@@ -1355,6 +1346,7 @@ static int do_set(int argc, char **argv)\n \n \t\t\tif (dev)\n \t\t\t\tduparg2(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", dev);\n \t\t\tdev = *argv;\n \t\t}\n \t\targc--; argv++;\n@@ -1383,9 +1375,6 @@ static int do_set(int argc, char **argv)\n \t}\n \n \tif (newname && strcmp(dev, newname)) {\n-\t\tif (strlen(newname) == 0)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t \"name\");\n \t\tif (do_changename(dev, newname) < 0)\n \t\t\treturn -1;\n \t\tdev = newname;\ndiff --git a/ip/ipmaddr.c b/ip/ipmaddr.c\nindex 85a69e779563d..282a06153c79a 100644\n--- a/ip/ipmaddr.c\n+++ b/ip/ipmaddr.c\n@@ -284,6 +284,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)\n \t\t\tNEXT_ARG();\n \t\t\tif (ifr.ifr_name[0])\n \t\t\t\tduparg(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tstrncpy(ifr.ifr_name, *argv, IFNAMSIZ);\n \t\t} else {\n \t\t\tif (matches(*argv, \"address\") == 0) {\ndiff --git a/ip/iprule.c b/ip/iprule.c\nindex 8313138db815f..33cfd87195212 100644\n--- a/ip/iprule.c\n+++ b/ip/iprule.c\n@@ -472,10 +472,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)\n \t\t} else if (strcmp(*argv, \"dev\") == 0 ||\n \t\t\t strcmp(*argv, \"iif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"iif\", *argv);\n \t\t\tstrncpy(filter.iif, *argv, IFNAMSIZ);\n \t\t\tfilter.iifmask = 1;\n \t\t} else if (strcmp(*argv, \"oif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"oif\", *argv);\n \t\t\tstrncpy(filter.oif, *argv, IFNAMSIZ);\n \t\t\tfilter.oifmask = 1;\n \t\t} else if (strcmp(*argv, \"l3mdev\") == 0) {\n@@ -695,10 +697,12 @@ static int iprule_modify(int cmd, int argc, char **argv)\n \t\t} else if (strcmp(*argv, \"dev\") == 0 ||\n \t\t\t strcmp(*argv, \"iif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev/iif\", *argv);\n \t\t\taddattr_l(&req.n, sizeof(req), FRA_IFNAME,\n \t\t\t\t *argv, strlen(*argv)+1);\n \t\t} else if (strcmp(*argv, \"oif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"oif\", *argv);\n \t\t\taddattr_l(&req.n, sizeof(req), FRA_OIFNAME,\n \t\t\t\t *argv, strlen(*argv)+1);\n \t\t} else if (strcmp(*argv, \"l3mdev\") == 0) {\ndiff --git a/ip/iptunnel.c b/ip/iptunnel.c\nindex 0acfd0793d3cd..851a80aad73a9 100644\n--- a/ip/iptunnel.c\n+++ b/ip/iptunnel.c\n@@ -178,7 +178,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)\n \n \t\t\tif (p->name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n-\t\t\tstrncpy(p->name, *argv, IFNAMSIZ - 1);\n+\t\t\tcheck_ifname(\"name\", *argv);\n+\t\t\tstrncpy(p->name, *argv, IFNAMSIZ);\n \t\t\tif (cmd == SIOCCHGTUNNEL && count == 0) {\n \t\t\t\tstruct ip_tunnel_parm old_p = {};\n \n@@ -487,6 +488,7 @@ static int do_prl(int argc, char **argv)\n \t\t\tcount++;\n \t\t} else if (strcmp(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tmedium = *argv;\n \t\t} else {\n \t\t\tfprintf(stderr,\n@@ -534,6 +536,7 @@ static int do_6rd(int argc, char **argv)\n \t\t\tcmd = SIOCDEL6RD;\n \t\t} else if (strcmp(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tmedium = *argv;\n \t\t} else {\n \t\t\tfprintf(stderr,\ndiff --git a/ip/iptuntap.c b/ip/iptuntap.c\nindex 451f7f0eac6bb..4400dc2fa2a88 100644\n--- a/ip/iptuntap.c\n+++ b/ip/iptuntap.c\n@@ -176,7 +176,8 @@ static int parse_args(int argc, char **argv,\n \t\t\tifr->ifr_flags |= IFF_MULTI_QUEUE;\n \t\t} else if (matches(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n-\t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n+\t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ);\n \t\t} else {\n \t\t\tif (matches(*argv, \"name\") == 0) {\n \t\t\t\tNEXT_ARG();\n@@ -184,6 +185,7 @@ static int parse_args(int argc, char **argv,\n \t\t\t\tusage();\n \t\t\tif (ifr->ifr_name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ);\n \t\t}\n \t\tcount++;\ndiff --git a/lib/utils.c b/lib/utils.c\nindex bbd3cbc46a0e5..c4a02b8f9f52a 100644\n--- a/lib/utils.c\n+++ b/lib/utils.c\n@@ -699,6 +699,16 @@ void duparg2(const char *key, const char *arg)\n \texit(-1);\n }\n \n+void check_ifname(const char *arg, const char *name)\n+{\n+\tsize_t len = strlen(name);\n+\n+\tif (!len)\n+\t\tinvarg(\"Empty interface name not allowed.\", arg);\n+\tif (len >= IFNAMSIZ)\n+\t\tinvarg(\"Interface name is too long.\", name);\n+}\n+\n int matches(const char *cmd, const char *pattern)\n {\n \tint len = strlen(cmd);\ndiff --git a/misc/arpd.c b/misc/arpd.c\nindex bfab44544ee1d..d42df9e58a9f1 100644\n--- a/misc/arpd.c\n+++ b/misc/arpd.c\n@@ -664,6 +664,7 @@ int main(int argc, char **argv)\n \t\tstruct ifreq ifr = {};\n \n \t\tfor (i = 0; i < ifnum; i++) {\n+\t\t\tcheck_ifname(ifnames[i], ifnames[i]);\n \t\t\tstrncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);\n \t\t\tif (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {\n \t\t\t\tperror(\"ioctl(SIOCGIFINDEX)\");\ndiff --git a/tc/f_flower.c b/tc/f_flower.c\nindex 99e62a382dec6..ff45ea7af086e 100644\n--- a/tc/f_flower.c\n+++ b/tc/f_flower.c\n@@ -630,6 +630,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,\n \t\t\tflags |= TCA_CLS_FLAGS_SKIP_SW;\n \t\t} else if (matches(*argv, \"indev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"indev\", *argv);\n \t\t\taddattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);\n \t\t} else if (matches(*argv, \"vlan_id\") == 0) {\n \t\t\t__u16 vid;\n", "prefixes": [ "iproute", "v2", "3/3" ] }