| Message ID | 20241129224816.2751362-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 77ac0b28c868c724675b4004d554f5938ce13693 |
| Headers | show |
| Series | [ovs-dev] netdev: Always clear struct ifreq before ioctl. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 29 Nov 2024, at 23:48, Ilya Maximets wrote: > It's not nice to send random stack memory to a kernel over ioctl: > > Uninitialized bytes in ioctl_common_pre > at offset 20 inside [0x7fff8899f3c0, 40) > WARNING: MemorySanitizer: use-of-uninitialized-value > 0 0x1180b7 in af_inet_ioctl lib/socket-util-unix.c:417:15 > 1 0x1180ff in af_inet_ifreq_ioctl lib/socket-util-unix.c:428:13 > 2 0x11e4fd in netdev_linux_set_mtu lib/netdev-linux.c:2005:13 > 3 0xba250b in netdev_set_mtu lib/netdev.c:1132:30 > 4 0x59a19f in update_mtu_ofproto ofproto/ofproto.c:3042:18 > 5 0x596947 in update_mtu ofproto/ofproto.c:3024:5 > 6 0x5976d6 in ofport_install ofproto/ofproto.c:2617:5 > 7 0x572e96 in update_port ofproto/ofproto.c:2893:21 > 8 0x57636b in ofproto_port_add ofproto/ofproto.c:2208:17 > 9 0x4f9e0b in iface_do_create vswitchd/bridge.c:2203:13 > 10 0x4f7f43 in iface_create vswitchd/bridge.c:2246:13 > 11 0x4f7a63 in bridge_add_ports__ vswitchd/bridge.c:1225:21 > 12 0x4d8ea4 in bridge_add_ports vswitchd/bridge.c:1241:5 > 13 0x4cce3a in bridge_reconfigure vswitchd/bridge.c:952:9 > 14 0x4ca156 in bridge_run vswitchd/bridge.c:3439:9 > 15 0x5278e7 in main vswitchd/ovs-vswitchd.c:137:9 > 16 0x7f9def in __libc_start_call_main > 17 0x7f9def in __libc_start_main@GLIBC_2.2.5 > 18 0x432b14 in _start (vswitchd/ovs-vswitchd+0x432b14) > > We do already initialize this structure for a few ioctls, let's > do that for all of them. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Thanks, I agree it would be good to be consistent. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 12/2/24 09:59, Eelco Chaudron wrote: > > > On 29 Nov 2024, at 23:48, Ilya Maximets wrote: > >> It's not nice to send random stack memory to a kernel over ioctl: >> >> Uninitialized bytes in ioctl_common_pre >> at offset 20 inside [0x7fff8899f3c0, 40) >> WARNING: MemorySanitizer: use-of-uninitialized-value >> 0 0x1180b7 in af_inet_ioctl lib/socket-util-unix.c:417:15 >> 1 0x1180ff in af_inet_ifreq_ioctl lib/socket-util-unix.c:428:13 >> 2 0x11e4fd in netdev_linux_set_mtu lib/netdev-linux.c:2005:13 >> 3 0xba250b in netdev_set_mtu lib/netdev.c:1132:30 >> 4 0x59a19f in update_mtu_ofproto ofproto/ofproto.c:3042:18 >> 5 0x596947 in update_mtu ofproto/ofproto.c:3024:5 >> 6 0x5976d6 in ofport_install ofproto/ofproto.c:2617:5 >> 7 0x572e96 in update_port ofproto/ofproto.c:2893:21 >> 8 0x57636b in ofproto_port_add ofproto/ofproto.c:2208:17 >> 9 0x4f9e0b in iface_do_create vswitchd/bridge.c:2203:13 >> 10 0x4f7f43 in iface_create vswitchd/bridge.c:2246:13 >> 11 0x4f7a63 in bridge_add_ports__ vswitchd/bridge.c:1225:21 >> 12 0x4d8ea4 in bridge_add_ports vswitchd/bridge.c:1241:5 >> 13 0x4cce3a in bridge_reconfigure vswitchd/bridge.c:952:9 >> 14 0x4ca156 in bridge_run vswitchd/bridge.c:3439:9 >> 15 0x5278e7 in main vswitchd/ovs-vswitchd.c:137:9 >> 16 0x7f9def in __libc_start_call_main >> 17 0x7f9def in __libc_start_main@GLIBC_2.2.5 >> 18 0x432b14 in _start (vswitchd/ovs-vswitchd+0x432b14) >> >> We do already initialize this structure for a few ioctls, let's >> do that for all of them. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > Thanks, I agree it would be good to be consistent. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Thanks! Applied and backported down to 3.3 for now. Best regards, Ilya Maximets.
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 8596741aa..6e3091b93 100644 --- a/lib/netdev-bsd.c +++ b/lib/netdev-bsd.c @@ -669,6 +669,7 @@ netdev_bsd_rxq_drain(struct netdev_rxq *rxq_) struct ifreq ifr; struct netdev_rxq_bsd *rxq = netdev_rxq_bsd_cast(rxq_); + memset(&ifr, 0, sizeof ifr); strcpy(ifr.ifr_name, netdev_get_kernel_name(netdev_rxq_get_netdev(rxq_))); if (ioctl(rxq->fd, BIOCFLUSH, &ifr) == -1) { VLOG_DBG_RL(&rl, "%s: ioctl(BIOCFLUSH) failed: %s", @@ -828,6 +829,7 @@ netdev_bsd_get_mtu(const struct netdev *netdev_, int *mtup) if (!(netdev->cache_valid & VALID_MTU)) { struct ifreq ifr; + memset(&ifr, 0, sizeof ifr); error = af_inet_ifreq_ioctl(netdev_get_kernel_name(netdev_), &ifr, SIOCGIFMTU, "SIOCGIFMTU"); if (!error) { @@ -1440,6 +1442,8 @@ do_set_addr(struct netdev *netdev, struct in_addr addr) { struct ifreq ifr; + + memset(&ifr, 0, sizeof ifr); make_in4_sockaddr(&ifr.ifr_addr, addr); return af_inet_ifreq_ioctl(netdev_get_kernel_name(netdev), &ifr, ioctl_nr, ioctl_name); @@ -1547,6 +1551,7 @@ destroy_tap(int fd, const char *name) struct ifreq ifr; close(fd); + memset(&ifr, 0, sizeof ifr); strcpy(ifr.ifr_name, name); /* XXX What to do if this call fails? */ af_inet_ioctl(SIOCIFDESTROY, &ifr); @@ -1558,6 +1563,7 @@ get_flags(const struct netdev *netdev, int *flags) struct ifreq ifr; int error; + memset(&ifr, 0, sizeof ifr); error = af_inet_ifreq_ioctl(netdev_get_kernel_name(netdev), &ifr, SIOCGIFFLAGS, "SIOCGIFFLAGS"); @@ -1571,6 +1577,7 @@ set_flags(const char *name, int flags) { struct ifreq ifr; + memset(&ifr, 0, sizeof ifr); ifr_set_flags(&ifr, flags); return af_inet_ifreq_ioctl(name, &ifr, SIOCSIFFLAGS, "SIOCSIFFLAGS"); diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 0cd0850a3..19bf62ece 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1028,6 +1028,8 @@ netdev_linux_construct_tap(struct netdev *netdev_) ovsthread_once_done(&once); } + memset(&ifr, 0, sizeof ifr); + ifr.ifr_flags = IFF_TAP | IFF_NO_PI; if (tap_supports_vnet_hdr) { ifr.ifr_flags |= IFF_VNET_HDR; @@ -1582,8 +1584,11 @@ netdev_linux_rxq_drain(struct netdev_rxq *rxq_) struct netdev_rxq_linux *rx = netdev_rxq_linux_cast(rxq_); if (rx->is_tap) { struct ifreq ifr; - int error = af_inet_ifreq_ioctl(netdev_rxq_get_name(rxq_), &ifr, - SIOCGIFTXQLEN, "SIOCGIFTXQLEN"); + int error; + + memset(&ifr, 0, sizeof ifr); + error = af_inet_ifreq_ioctl(netdev_rxq_get_name(rxq_), &ifr, + SIOCGIFTXQLEN, "SIOCGIFTXQLEN"); if (error) { return error; } @@ -1939,6 +1944,7 @@ netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup) /* Fall back to ioctl if netlink fails */ struct ifreq ifr; + memset(&ifr, 0, sizeof ifr); netdev->netdev_mtu_error = af_inet_ifreq_ioctl( netdev_get_name(&netdev->up), &ifr, SIOCGIFMTU, "SIOCGIFMTU"); netdev->mtu = ifr.ifr_mtu; @@ -2001,7 +2007,10 @@ netdev_linux_set_mtu(struct netdev *netdev_, int mtu) } netdev->cache_valid &= ~VALID_MTU; } + + memset(&ifr, 0, sizeof ifr); ifr.ifr_mtu = mtu; + error = af_inet_ifreq_ioctl(netdev_get_name(netdev_), &ifr, SIOCSIFMTU, "SIOCSIFMTU"); if (!error || error == ENODEV) { @@ -3570,6 +3579,7 @@ do_set_addr(struct netdev *netdev, { struct ifreq ifr; + memset(&ifr, 0, sizeof ifr); make_in4_sockaddr(&ifr.ifr_addr, addr); return af_inet_ifreq_ioctl(netdev_get_name(netdev), &ifr, ioctl_nr, ioctl_name); @@ -6767,6 +6777,7 @@ get_flags(const struct netdev *dev, unsigned int *flags) struct ifreq ifr; int error; + memset(&ifr, 0, sizeof ifr); *flags = 0; error = af_inet_ifreq_ioctl(dev->name, &ifr, SIOCGIFFLAGS, "SIOCGIFFLAGS"); if (!error) { @@ -6780,6 +6791,7 @@ set_flags(const char *name, unsigned int flags) { struct ifreq ifr; + memset(&ifr, 0, sizeof ifr); ifr.ifr_flags = flags; return af_inet_ifreq_ioctl(name, &ifr, SIOCSIFFLAGS, "SIOCSIFFLAGS"); } @@ -6790,6 +6802,7 @@ linux_get_ifindex(const char *netdev_name) struct ifreq ifr; int error; + memset(&ifr, 0, sizeof ifr); ovs_strzcpy(ifr.ifr_name, netdev_name, sizeof ifr.ifr_name); COVERAGE_INC(netdev_get_ifindex);
It's not nice to send random stack memory to a kernel over ioctl: Uninitialized bytes in ioctl_common_pre at offset 20 inside [0x7fff8899f3c0, 40) WARNING: MemorySanitizer: use-of-uninitialized-value 0 0x1180b7 in af_inet_ioctl lib/socket-util-unix.c:417:15 1 0x1180ff in af_inet_ifreq_ioctl lib/socket-util-unix.c:428:13 2 0x11e4fd in netdev_linux_set_mtu lib/netdev-linux.c:2005:13 3 0xba250b in netdev_set_mtu lib/netdev.c:1132:30 4 0x59a19f in update_mtu_ofproto ofproto/ofproto.c:3042:18 5 0x596947 in update_mtu ofproto/ofproto.c:3024:5 6 0x5976d6 in ofport_install ofproto/ofproto.c:2617:5 7 0x572e96 in update_port ofproto/ofproto.c:2893:21 8 0x57636b in ofproto_port_add ofproto/ofproto.c:2208:17 9 0x4f9e0b in iface_do_create vswitchd/bridge.c:2203:13 10 0x4f7f43 in iface_create vswitchd/bridge.c:2246:13 11 0x4f7a63 in bridge_add_ports__ vswitchd/bridge.c:1225:21 12 0x4d8ea4 in bridge_add_ports vswitchd/bridge.c:1241:5 13 0x4cce3a in bridge_reconfigure vswitchd/bridge.c:952:9 14 0x4ca156 in bridge_run vswitchd/bridge.c:3439:9 15 0x5278e7 in main vswitchd/ovs-vswitchd.c:137:9 16 0x7f9def in __libc_start_call_main 17 0x7f9def in __libc_start_main@GLIBC_2.2.5 18 0x432b14 in _start (vswitchd/ovs-vswitchd+0x432b14) We do already initialize this structure for a few ioctls, let's do that for all of them. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/netdev-bsd.c | 7 +++++++ lib/netdev-linux.c | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)