| Message ID | 20190723120810.28801-1-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] libbpf: fix using uninitialized ioctl results | expand |
On Tue, Jul 23, 2019 at 5:08 AM Ilya Maximets <i.maximets@samsung.com> wrote: > > 'channels.max_combined' initialized only on ioctl success and > errno is only valid on ioctl failure. > > The code doesn't produce any runtime issues, but makes memory > sanitizers angry: > > Conditional jump or move depends on uninitialised value(s) > at 0x55C056F: xsk_get_max_queues (xsk.c:336) > by 0x55C05B2: xsk_create_bpf_maps (xsk.c:354) > by 0x55C089F: xsk_setup_xdp_prog (xsk.c:447) > by 0x55C0E57: xsk_socket__create (xsk.c:601) > Uninitialised value was created by a stack allocation > at 0x55C04CD: xsk_get_max_queues (xsk.c:318) > > Additionally fixed warning on uninitialized bytes in ioctl arguments: > > Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s) > at 0x648D45B: ioctl (in /usr/lib64/libc-2.28.so) > by 0x55C0546: xsk_get_max_queues (xsk.c:330) > by 0x55C05B2: xsk_create_bpf_maps (xsk.c:354) > by 0x55C089F: xsk_setup_xdp_prog (xsk.c:447) > by 0x55C0E57: xsk_socket__create (xsk.c:601) > Address 0x1ffefff378 is on thread 1's stack > in frame #1, created by xsk_get_max_queues (xsk.c:318) > Uninitialised value was created by a stack allocation > at 0x55C04CD: xsk_get_max_queues (xsk.c:318) > > CC: Magnus Karlsson <magnus.karlsson@intel.com> > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > tools/lib/bpf/xsk.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 5007b5d4fd2c..c4f912dc30f9 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -317,7 +317,7 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) > > static int xsk_get_max_queues(struct xsk_socket *xsk) > { > - struct ethtool_channels channels; > + struct ethtool_channels channels = { .cmd = ETHTOOL_GCHANNELS }; > struct ifreq ifr; > int fd, err, ret; > > @@ -325,7 +325,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > if (fd < 0) > return -errno; > > - channels.cmd = ETHTOOL_GCHANNELS; > + memset(&ifr, 0, sizeof(ifr)); I refactored this bit into struct ifreq ifr = {}; based on Andrii suggestion and pushed to bpf tree. Thanks > ifr.ifr_data = (void *)&channels; > strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1); > ifr.ifr_name[IFNAMSIZ - 1] = '\0'; > @@ -335,7 +335,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > goto out; > } > > - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > + if (err || channels.max_combined == 0) > /* If the device says it has no channels, then all traffic > * is sent to a single stream, so max queues = 1. > */ > -- > 2.17.1 >
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 5007b5d4fd2c..c4f912dc30f9 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -317,7 +317,7 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) static int xsk_get_max_queues(struct xsk_socket *xsk) { - struct ethtool_channels channels; + struct ethtool_channels channels = { .cmd = ETHTOOL_GCHANNELS }; struct ifreq ifr; int fd, err, ret; @@ -325,7 +325,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) if (fd < 0) return -errno; - channels.cmd = ETHTOOL_GCHANNELS; + memset(&ifr, 0, sizeof(ifr)); ifr.ifr_data = (void *)&channels; strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1); ifr.ifr_name[IFNAMSIZ - 1] = '\0'; @@ -335,7 +335,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) goto out; } - if (channels.max_combined == 0 || errno == EOPNOTSUPP) + if (err || channels.max_combined == 0) /* If the device says it has no channels, then all traffic * is sent to a single stream, so max queues = 1. */
'channels.max_combined' initialized only on ioctl success and errno is only valid on ioctl failure. The code doesn't produce any runtime issues, but makes memory sanitizers angry: Conditional jump or move depends on uninitialised value(s) at 0x55C056F: xsk_get_max_queues (xsk.c:336) by 0x55C05B2: xsk_create_bpf_maps (xsk.c:354) by 0x55C089F: xsk_setup_xdp_prog (xsk.c:447) by 0x55C0E57: xsk_socket__create (xsk.c:601) Uninitialised value was created by a stack allocation at 0x55C04CD: xsk_get_max_queues (xsk.c:318) Additionally fixed warning on uninitialized bytes in ioctl arguments: Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s) at 0x648D45B: ioctl (in /usr/lib64/libc-2.28.so) by 0x55C0546: xsk_get_max_queues (xsk.c:330) by 0x55C05B2: xsk_create_bpf_maps (xsk.c:354) by 0x55C089F: xsk_setup_xdp_prog (xsk.c:447) by 0x55C0E57: xsk_socket__create (xsk.c:601) Address 0x1ffefff378 is on thread 1's stack in frame #1, created by xsk_get_max_queues (xsk.c:318) Uninitialised value was created by a stack allocation at 0x55C04CD: xsk_get_max_queues (xsk.c:318) CC: Magnus Karlsson <magnus.karlsson@intel.com> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- tools/lib/bpf/xsk.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)