From 08992b2078f673b2806e0be30d07f41012a650e7 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Mon, 19 Jun 2023 10:37:52 -0700 Subject: [PATCH] ifconfig: Avoid issues with trying to negate unsigned values. The if_flags and if_cap fields hold a bitmask of flags. If a flag is the MSB of the field, then the logic in setifflags and setifcap which uses a < 0 check does the wrong thing (it tries to clear the flag rather than setting it). Also, trying to use - doesn't actually work as the result is a nop. To fix, stop overloading setifcap and setifflags and instead add new dedicated action functions clearifcap and clearifflags for clearing a flag. The value passed in the argument to the command is now always the raw flag. This was reported by a GCC warning after raising WARNS: sbin/ifconfig/ifconfig.c:2061:33: error: integer overflow in expression '-2147483648' of type 'int' results in '-2147483648' [-Werror=overflow] 2061 | DEF_CMD("-txtlsrtlmt", -IFCAP_TXTLS_RTLMT, setifcap), | ^ Reviewed by: emaste Differential Revision: https://reviews.freebsd.org/D40608 --- sbin/ifconfig/ifconfig.c | 113 ++++++++++++++++++++++++--------------- sbin/ifconfig/ifconfig.h | 1 + sbin/ifconfig/ifvlan.c | 10 ++-- sbin/ifconfig/ifvxlan.c | 4 +- 4 files changed, 79 insertions(+), 49 deletions(-) diff --git a/sbin/ifconfig/ifconfig.c b/sbin/ifconfig/ifconfig.c index 6d66677467c1..5d789795e636 100644 --- a/sbin/ifconfig/ifconfig.c +++ b/sbin/ifconfig/ifconfig.c @@ -1396,6 +1396,22 @@ getifflags(const char *ifname, int us, bool err_ok) * of the ifreq structure, which may confuse other parts of ifconfig. * Make a private copy so we can avoid that. */ +static void +clearifflags(if_ctx *ctx, const char *vname, int value) +{ + struct ifreq my_ifr; + int flags; + + flags = getifflags(ctx->ifname, ctx->io_s, false); + flags &= ~value; + memset(&my_ifr, 0, sizeof(my_ifr)); + strlcpy(my_ifr.ifr_name, ctx->ifname, sizeof(my_ifr.ifr_name)); + my_ifr.ifr_flags = flags & 0xffff; + my_ifr.ifr_flagshigh = flags >> 16; + if (ioctl(ctx->io_s, SIOCSIFFLAGS, (caddr_t)&my_ifr) < 0) + Perror(vname); +} + static void setifflags(if_ctx *ctx, const char *vname, int value) { @@ -1403,11 +1419,7 @@ setifflags(if_ctx *ctx, const char *vname, int value) int flags; flags = getifflags(ctx->ifname, ctx->io_s, false); - if (value < 0) { - value = -value; - flags &= ~value; - } else - flags |= value; + flags |= value; memset(&my_ifr, 0, sizeof(my_ifr)); strlcpy(my_ifr.ifr_name, ctx->ifname, sizeof(my_ifr.ifr_name)); my_ifr.ifr_flags = flags & 0xffff; @@ -1416,6 +1428,27 @@ setifflags(if_ctx *ctx, const char *vname, int value) Perror(vname); } +void +clearifcap(if_ctx *ctx, const char *vname, int value) +{ + struct ifreq ifr = {}; + int flags; + + if (ioctl_ctx_ifr(ctx, SIOCGIFCAP, &ifr) < 0) { + Perror("ioctl (SIOCGIFCAP)"); + exit(1); + } + flags = ifr.ifr_curcap; + flags &= ~value; + flags &= ifr.ifr_reqcap; + /* Check for no change in capabilities. */ + if (ifr.ifr_curcap == flags) + return; + ifr.ifr_reqcap = flags; + if (ioctl_ctx(ctx, SIOCSIFCAP, &ifr) < 0) + Perror(vname); +} + void setifcap(if_ctx *ctx, const char *vname, int value) { @@ -1427,11 +1460,7 @@ setifcap(if_ctx *ctx, const char *vname, int value) exit(1); } flags = ifr.ifr_curcap; - if (value < 0) { - value = -value; - flags &= ~value; - } else - flags |= value; + flags |= value; flags &= ifr.ifr_reqcap; /* Check for no change in capabilities. */ if (ifr.ifr_curcap == flags) @@ -1972,17 +2001,17 @@ ifmaybeload(struct ifconfig_args *args, const char *name) static struct cmd basic_cmds[] = { DEF_CMD("up", IFF_UP, setifflags), - DEF_CMD("down", -IFF_UP, setifflags), - DEF_CMD("arp", -IFF_NOARP, setifflags), + DEF_CMD("down", IFF_UP, clearifflags), + DEF_CMD("arp", IFF_NOARP, clearifflags), DEF_CMD("-arp", IFF_NOARP, setifflags), DEF_CMD("debug", IFF_DEBUG, setifflags), - DEF_CMD("-debug", -IFF_DEBUG, setifflags), + DEF_CMD("-debug", IFF_DEBUG, clearifflags), DEF_CMD_ARG("description", setifdescr), DEF_CMD_ARG("descr", setifdescr), DEF_CMD("-description", 0, unsetifdescr), DEF_CMD("-descr", 0, unsetifdescr), DEF_CMD("promisc", IFF_PPROMISC, setifflags), - DEF_CMD("-promisc", -IFF_PPROMISC, setifflags), + DEF_CMD("-promisc", IFF_PPROMISC, clearifflags), DEF_CMD("add", IFF_UP, notealias), DEF_CMD("alias", IFF_UP, notealias), DEF_CMD("-alias", -IFF_UP, notealias), @@ -1991,7 +2020,7 @@ static struct cmd basic_cmds[] = { #ifdef notdef #define EN_SWABIPS 0x1000 DEF_CMD("swabips", EN_SWABIPS, setifflags), - DEF_CMD("-swabips", -EN_SWABIPS, setifflags), + DEF_CMD("-swabips", EN_SWABIPS, clearifflags), #endif DEF_CMD_ARG("netmask", setifnetmask), DEF_CMD_ARG("metric", setifmetric), @@ -2004,64 +2033,64 @@ static struct cmd basic_cmds[] = { DEF_CMD_ARG("-vnet", setifrvnet), #endif DEF_CMD("link0", IFF_LINK0, setifflags), - DEF_CMD("-link0", -IFF_LINK0, setifflags), + DEF_CMD("-link0", IFF_LINK0, clearifflags), DEF_CMD("link1", IFF_LINK1, setifflags), - DEF_CMD("-link1", -IFF_LINK1, setifflags), + DEF_CMD("-link1", IFF_LINK1, clearifflags), DEF_CMD("link2", IFF_LINK2, setifflags), - DEF_CMD("-link2", -IFF_LINK2, setifflags), + DEF_CMD("-link2", IFF_LINK2, clearifflags), DEF_CMD("monitor", IFF_MONITOR, setifflags), - DEF_CMD("-monitor", -IFF_MONITOR, setifflags), + DEF_CMD("-monitor", IFF_MONITOR, clearifflags), DEF_CMD("mextpg", IFCAP_MEXTPG, setifcap), - DEF_CMD("-mextpg", -IFCAP_MEXTPG, setifcap), + DEF_CMD("-mextpg", IFCAP_MEXTPG, clearifcap), DEF_CMD("staticarp", IFF_STATICARP, setifflags), - DEF_CMD("-staticarp", -IFF_STATICARP, setifflags), + DEF_CMD("-staticarp", IFF_STATICARP, clearifflags), DEF_CMD("stickyarp", IFF_STICKYARP, setifflags), - DEF_CMD("-stickyarp", -IFF_STICKYARP, setifflags), + DEF_CMD("-stickyarp", IFF_STICKYARP, clearifflags), DEF_CMD("rxcsum6", IFCAP_RXCSUM_IPV6, setifcap), - DEF_CMD("-rxcsum6", -IFCAP_RXCSUM_IPV6, setifcap), + DEF_CMD("-rxcsum6", IFCAP_RXCSUM_IPV6, clearifcap), DEF_CMD("txcsum6", IFCAP_TXCSUM_IPV6, setifcap), - DEF_CMD("-txcsum6", -IFCAP_TXCSUM_IPV6, setifcap), + DEF_CMD("-txcsum6", IFCAP_TXCSUM_IPV6, clearifcap), DEF_CMD("rxcsum", IFCAP_RXCSUM, setifcap), - DEF_CMD("-rxcsum", -IFCAP_RXCSUM, setifcap), + DEF_CMD("-rxcsum", IFCAP_RXCSUM, clearifcap), DEF_CMD("txcsum", IFCAP_TXCSUM, setifcap), - DEF_CMD("-txcsum", -IFCAP_TXCSUM, setifcap), + DEF_CMD("-txcsum", IFCAP_TXCSUM, clearifcap), DEF_CMD("netcons", IFCAP_NETCONS, setifcap), - DEF_CMD("-netcons", -IFCAP_NETCONS, setifcap), + DEF_CMD("-netcons", IFCAP_NETCONS, clearifcap), DEF_CMD_ARG("pcp", setifpcp), DEF_CMD("-pcp", 0, disableifpcp), DEF_CMD("polling", IFCAP_POLLING, setifcap), - DEF_CMD("-polling", -IFCAP_POLLING, setifcap), + DEF_CMD("-polling", IFCAP_POLLING, clearifcap), DEF_CMD("tso6", IFCAP_TSO6, setifcap), - DEF_CMD("-tso6", -IFCAP_TSO6, setifcap), + DEF_CMD("-tso6", IFCAP_TSO6, clearifcap), DEF_CMD("tso4", IFCAP_TSO4, setifcap), - DEF_CMD("-tso4", -IFCAP_TSO4, setifcap), + DEF_CMD("-tso4", IFCAP_TSO4, clearifcap), DEF_CMD("tso", IFCAP_TSO, setifcap), - DEF_CMD("-tso", -IFCAP_TSO, setifcap), + DEF_CMD("-tso", IFCAP_TSO, clearifcap), DEF_CMD("toe", IFCAP_TOE, setifcap), - DEF_CMD("-toe", -IFCAP_TOE, setifcap), + DEF_CMD("-toe", IFCAP_TOE, clearifcap), DEF_CMD("lro", IFCAP_LRO, setifcap), - DEF_CMD("-lro", -IFCAP_LRO, setifcap), + DEF_CMD("-lro", IFCAP_LRO, clearifcap), DEF_CMD("txtls", IFCAP_TXTLS, setifcap), - DEF_CMD("-txtls", -IFCAP_TXTLS, setifcap), + DEF_CMD("-txtls", IFCAP_TXTLS, clearifcap), DEF_CMD_SARG("rxtls", IFCAP2_RXTLS4_NAME "," IFCAP2_RXTLS6_NAME, setifcapnv), DEF_CMD_SARG("-rxtls", "-"IFCAP2_RXTLS4_NAME ",-" IFCAP2_RXTLS6_NAME, setifcapnv), DEF_CMD("wol", IFCAP_WOL, setifcap), - DEF_CMD("-wol", -IFCAP_WOL, setifcap), + DEF_CMD("-wol", IFCAP_WOL, clearifcap), DEF_CMD("wol_ucast", IFCAP_WOL_UCAST, setifcap), - DEF_CMD("-wol_ucast", -IFCAP_WOL_UCAST, setifcap), + DEF_CMD("-wol_ucast", IFCAP_WOL_UCAST, clearifcap), DEF_CMD("wol_mcast", IFCAP_WOL_MCAST, setifcap), - DEF_CMD("-wol_mcast", -IFCAP_WOL_MCAST, setifcap), + DEF_CMD("-wol_mcast", IFCAP_WOL_MCAST, clearifcap), DEF_CMD("wol_magic", IFCAP_WOL_MAGIC, setifcap), - DEF_CMD("-wol_magic", -IFCAP_WOL_MAGIC, setifcap), + DEF_CMD("-wol_magic", IFCAP_WOL_MAGIC, clearifcap), DEF_CMD("txrtlmt", IFCAP_TXRTLMT, setifcap), - DEF_CMD("-txrtlmt", -IFCAP_TXRTLMT, setifcap), + DEF_CMD("-txrtlmt", IFCAP_TXRTLMT, clearifcap), DEF_CMD("txtlsrtlmt", IFCAP_TXTLS_RTLMT, setifcap), - DEF_CMD("-txtlsrtlmt", -IFCAP_TXTLS_RTLMT, setifcap), + DEF_CMD("-txtlsrtlmt", IFCAP_TXTLS_RTLMT, clearifcap), DEF_CMD("hwrxtstmp", IFCAP_HWRXTSTMP, setifcap), - DEF_CMD("-hwrxtstmp", -IFCAP_HWRXTSTMP, setifcap), - DEF_CMD("normal", -IFF_LINK0, setifflags), + DEF_CMD("-hwrxtstmp", IFCAP_HWRXTSTMP, clearifcap), + DEF_CMD("normal", IFF_LINK0, clearifflags), DEF_CMD("compress", IFF_LINK0, setifflags), DEF_CMD("noicmp", IFF_LINK1, setifflags), DEF_CMD_ARG("mtu", setifmtu), diff --git a/sbin/ifconfig/ifconfig.h b/sbin/ifconfig/ifconfig.h index e33a2c63aec1..84c1ac7eebce 100644 --- a/sbin/ifconfig/ifconfig.h +++ b/sbin/ifconfig/ifconfig.h @@ -255,6 +255,7 @@ extern int allmedia; extern int exit_code; extern char *f_inet, *f_inet6, *f_ether, *f_addr; +void clearifcap(if_ctx *ctx, const char *, int value); void setifcap(if_ctx *ctx, const char *, int value); void setifcapnv(if_ctx *ctx, const char *vname, const char *arg); diff --git a/sbin/ifconfig/ifvlan.c b/sbin/ifconfig/ifvlan.c index b871e953c44a..adc3c4692f8b 100644 --- a/sbin/ifconfig/ifvlan.c +++ b/sbin/ifconfig/ifvlan.c @@ -286,15 +286,15 @@ static struct cmd vlan_cmds[] = { /* XXX For compatibility. Should become DEF_CMD() some day. */ DEF_CMD_OPTARG("-vlandev", unsetvlandev), DEF_CMD("vlanmtu", IFCAP_VLAN_MTU, setifcap), - DEF_CMD("-vlanmtu", -IFCAP_VLAN_MTU, setifcap), + DEF_CMD("-vlanmtu", IFCAP_VLAN_MTU, clearifcap), DEF_CMD("vlanhwtag", IFCAP_VLAN_HWTAGGING, setifcap), - DEF_CMD("-vlanhwtag", -IFCAP_VLAN_HWTAGGING, setifcap), + DEF_CMD("-vlanhwtag", IFCAP_VLAN_HWTAGGING, clearifcap), DEF_CMD("vlanhwfilter", IFCAP_VLAN_HWFILTER, setifcap), - DEF_CMD("-vlanhwfilter", -IFCAP_VLAN_HWFILTER, setifcap), - DEF_CMD("-vlanhwtso", -IFCAP_VLAN_HWTSO, setifcap), + DEF_CMD("-vlanhwfilter", IFCAP_VLAN_HWFILTER, clearifcap), + DEF_CMD("-vlanhwtso", IFCAP_VLAN_HWTSO, clearifcap), DEF_CMD("vlanhwtso", IFCAP_VLAN_HWTSO, setifcap), DEF_CMD("vlanhwcsum", IFCAP_VLAN_HWCSUM, setifcap), - DEF_CMD("-vlanhwcsum", -IFCAP_VLAN_HWCSUM, setifcap), + DEF_CMD("-vlanhwcsum", IFCAP_VLAN_HWCSUM, clearifcap), }; static struct afswtch af_vlan = { .af_name = "af_vlan", diff --git a/sbin/ifconfig/ifvxlan.c b/sbin/ifconfig/ifvxlan.c index 4f54bee88b41..72e2eac35c2d 100644 --- a/sbin/ifconfig/ifvxlan.c +++ b/sbin/ifconfig/ifvxlan.c @@ -613,9 +613,9 @@ static struct cmd vxlan_cmds[] = { DEF_CMD("vxlanflushall", 1, setvxlan_flush), DEF_CMD("vxlanhwcsum", IFCAP_VXLAN_HWCSUM, setifcap), - DEF_CMD("-vxlanhwcsum", -IFCAP_VXLAN_HWCSUM, setifcap), + DEF_CMD("-vxlanhwcsum", IFCAP_VXLAN_HWCSUM, clearifcap), DEF_CMD("vxlanhwtso", IFCAP_VXLAN_HWTSO, setifcap), - DEF_CMD("-vxlanhwtso", -IFCAP_VXLAN_HWTSO, setifcap), + DEF_CMD("-vxlanhwtso", IFCAP_VXLAN_HWTSO, clearifcap), }; static struct afswtch af_vxlan = {