| Message ID | 20180131003111.3492-1-daniel@iogearbox.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: fix null pointer deref in bpf_prog_test_run_xdp | expand |
On Wed, 31 Jan 2018 01:31:11 +0100 Daniel Borkmann <daniel@iogearbox.net> wrote: > syzkaller was able to generate the following XDP program ... > > (18) r0 = 0x0 > (61) r5 = *(u32 *)(r1 +12) > (04) (u32) r0 += (u32) 0 > (95) exit > > ... and trigger a NULL pointer dereference in ___bpf_prog_run() > via bpf_prog_test_run_xdp() where this was attempted to run. > > Reason is that recent xdp_rxq_info addition to XDP programs > updated all drivers, but not bpf_prog_test_run_xdp(), where > xdp_buff is set up. Thus when context rewriter does the deref > on the netdev it's NULL at runtime. Fix it by using xdp_rxq > from loopback dev. netif_get_rx_queue() helper can also be > reused in various other locations later on. > > Fixes: 02dd3291b2f0 ("bpf: finally expose xdp_rxq_info to XDP bpf-programs") > Reported-by: syzbot+1eb094057b338eb1fc00@syzkaller.appspotmail.com > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Jesper Dangaard Brouer <brouer@redhat.com> > --- > [ Note: Needs to wait till Linus pulled Dave's net-next so > the affected commit lands in bpf tree. ] > > include/linux/netdevice.h | 6 ++++++ > net/bpf/test_run.c | 4 ++++ > tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index cd46d3d..9630f4e 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -3228,6 +3228,12 @@ static inline int netif_set_real_num_rx_queues(struct net_device *dev, > } > #endif > > +static inline struct netdev_rx_queue * > +netif_get_rx_queue(struct net_device *dev, unsigned int rxq) This function name is very close to (net/core/dev.c): static struct netdev_rx_queue * netif_get_rxqueue(struct sk_buff *skb) > +{ > + return dev->_rx + rxq; > +} I know above is correct, is just annoys my eyes as I find it more natural to write as dev->_rx[rxq]. I'm not saying you should change it, because it does preserve the access style elsewhere (and I also kept this style in some of my changes). You mention netif_get_rx_queue() helper can also be reused in various other locations later on, but there is not boundary checks (on this dynamically allocated array of size dev->num_rx_queues). We could add a comment saying caller is responsible for boundary checks, or prefix the function name with "__" to indicate this indirectly. > #ifdef CONFIG_SYSFS > static inline unsigned int get_netdev_rx_queue_index( > struct netdev_rx_queue *queue) > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index a86e668..47cd0b7 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -151,6 +151,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > { > u32 size = kattr->test.data_size_in; > u32 repeat = kattr->test.repeat; > + struct netdev_rx_queue *rxqueue; > struct xdp_buff xdp = {}; > u32 retval, duration; > void *data; > @@ -165,6 +166,9 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > xdp.data_meta = xdp.data; > xdp.data_end = xdp.data + size; > > + rxqueue = netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); > + xdp.rxq = &rxqueue->xdp_rxq; > + > retval = bpf_test_run(prog, &xdp, repeat, &duration); > if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN) > size = xdp.data_end - xdp.data; > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index 697bd83..c0f16e9 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -7780,6 +7780,20 @@ static struct bpf_test tests[] = { > .result = REJECT, > }, > { > + "XDP, using ifindex from netdev", > + .insns = { > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, > + offsetof(struct xdp_md, ingress_ifindex)), > + BPF_JMP_IMM(BPF_JLT, BPF_REG_2, 1, 1), > + BPF_MOV64_IMM(BPF_REG_0, 1), > + BPF_EXIT_INSN(), > + }, > + .result = ACCEPT, > + .prog_type = BPF_PROG_TYPE_XDP, > + .retval = 1, > + }, > + { > "meta access, test1", > .insns = { > BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
On 01/31/2018 08:24 AM, Jesper Dangaard Brouer wrote: > On Wed, 31 Jan 2018 01:31:11 +0100 > Daniel Borkmann <daniel@iogearbox.net> wrote: > >> syzkaller was able to generate the following XDP program ... >> >> (18) r0 = 0x0 >> (61) r5 = *(u32 *)(r1 +12) >> (04) (u32) r0 += (u32) 0 >> (95) exit >> >> ... and trigger a NULL pointer dereference in ___bpf_prog_run() >> via bpf_prog_test_run_xdp() where this was attempted to run. >> >> Reason is that recent xdp_rxq_info addition to XDP programs >> updated all drivers, but not bpf_prog_test_run_xdp(), where >> xdp_buff is set up. Thus when context rewriter does the deref >> on the netdev it's NULL at runtime. Fix it by using xdp_rxq >> from loopback dev. netif_get_rx_queue() helper can also be >> reused in various other locations later on. >> >> Fixes: 02dd3291b2f0 ("bpf: finally expose xdp_rxq_info to XDP bpf-programs") >> Reported-by: syzbot+1eb094057b338eb1fc00@syzkaller.appspotmail.com >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> >> Cc: Jesper Dangaard Brouer <brouer@redhat.com> >> --- >> [ Note: Needs to wait till Linus pulled Dave's net-next so >> the affected commit lands in bpf tree. ] >> >> include/linux/netdevice.h | 6 ++++++ >> net/bpf/test_run.c | 4 ++++ >> tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index cd46d3d..9630f4e 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -3228,6 +3228,12 @@ static inline int netif_set_real_num_rx_queues(struct net_device *dev, >> } >> #endif >> >> +static inline struct netdev_rx_queue * >> +netif_get_rx_queue(struct net_device *dev, unsigned int rxq) > > This function name is very close to (net/core/dev.c): > > static struct netdev_rx_queue * > netif_get_rxqueue(struct sk_buff *skb) > > >> +{ >> + return dev->_rx + rxq; >> +} > > I know above is correct, is just annoys my eyes as I find it more > natural to write as dev->_rx[rxq]. I'm not saying you should change > it, because it does preserve the access style elsewhere (and I also > kept this style in some of my changes). > > You mention netif_get_rx_queue() helper can also be reused in various > other locations later on, but there is not boundary checks (on this From a quick grep, few more could use this (didn't add this into the fix here, though, but can be later as cleanup): $ git grep -n "dev->_rx +" net/core/dev.c:3644: rxqueue = dev->_rx + rxq_index; net/core/dev.c:3784: struct netdev_rx_queue *rxqueue = dev->_rx + rxq_index; net/core/net-sysfs.c:940: struct netdev_rx_queue *queue = dev->_rx + index; > dynamically allocated array of size dev->num_rx_queues). We could add > a comment saying caller is responsible for boundary checks, or prefix > the function name with "__" to indicate this indirectly. Okay, I don't mind changing the name, would __netif_get_rx_queue() be a better one? Thanks, Daniel
On Wed, 31 Jan 2018 11:42:16 +0100 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 01/31/2018 08:24 AM, Jesper Dangaard Brouer wrote: > > On Wed, 31 Jan 2018 01:31:11 +0100 > > Daniel Borkmann <daniel@iogearbox.net> wrote: > > > >> syzkaller was able to generate the following XDP program ... > >> > >> (18) r0 = 0x0 > >> (61) r5 = *(u32 *)(r1 +12) > >> (04) (u32) r0 += (u32) 0 > >> (95) exit > >> > >> ... and trigger a NULL pointer dereference in ___bpf_prog_run() > >> via bpf_prog_test_run_xdp() where this was attempted to run. > >> > >> Reason is that recent xdp_rxq_info addition to XDP programs > >> updated all drivers, but not bpf_prog_test_run_xdp(), where > >> xdp_buff is set up. Thus when context rewriter does the deref > >> on the netdev it's NULL at runtime. Fix it by using xdp_rxq > >> from loopback dev. netif_get_rx_queue() helper can also be > >> reused in various other locations later on. > >> > >> Fixes: 02dd3291b2f0 ("bpf: finally expose xdp_rxq_info to XDP bpf-programs") > >> Reported-by: syzbot+1eb094057b338eb1fc00@syzkaller.appspotmail.com > >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > >> Cc: Jesper Dangaard Brouer <brouer@redhat.com> > >> --- > >> [ Note: Needs to wait till Linus pulled Dave's net-next so > >> the affected commit lands in bpf tree. ] > >> > >> include/linux/netdevice.h | 6 ++++++ > >> net/bpf/test_run.c | 4 ++++ > >> tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ > >> 3 files changed, 24 insertions(+) > >> > >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >> index cd46d3d..9630f4e 100644 > >> --- a/include/linux/netdevice.h > >> +++ b/include/linux/netdevice.h > >> @@ -3228,6 +3228,12 @@ static inline int netif_set_real_num_rx_queues(struct net_device *dev, > >> } > >> #endif > >> > >> +static inline struct netdev_rx_queue * > >> +netif_get_rx_queue(struct net_device *dev, unsigned int rxq) > > > > This function name is very close to (net/core/dev.c): > > > > static struct netdev_rx_queue * > > netif_get_rxqueue(struct sk_buff *skb) > > > > > >> +{ > >> + return dev->_rx + rxq; > >> +} > > > > I know above is correct, is just annoys my eyes as I find it more > > natural to write as dev->_rx[rxq]. I'm not saying you should change > > it, because it does preserve the access style elsewhere (and I also > > kept this style in some of my changes). > > > > You mention netif_get_rx_queue() helper can also be reused in various > > other locations later on, but there is not boundary checks (on this > > From a quick grep, few more could use this (didn't add this into > the fix here, though, but can be later as cleanup): Definitely save for a later cleanup, given the merge window timing. > $ git grep -n "dev->_rx +" > net/core/dev.c:3644: rxqueue = dev->_rx + rxq_index; > net/core/dev.c:3784: struct netdev_rx_queue *rxqueue = dev->_rx + rxq_index; > net/core/net-sysfs.c:940: struct netdev_rx_queue *queue = dev->_rx + index; > > > dynamically allocated array of size dev->num_rx_queues). We could add > > a comment saying caller is responsible for boundary checks, or prefix > > the function name with "__" to indicate this indirectly. > > Okay, I don't mind changing the name, would __netif_get_rx_queue() > be a better one? Yes, agreed. (BTW - naming of function netif_get_rxqueue (in dev.c) is in principle wrong it should have been netif_get_rx_queue I guess, but such nitpick cleanup should also be postponed or ignored ;-))
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index cd46d3d..9630f4e 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3228,6 +3228,12 @@ static inline int netif_set_real_num_rx_queues(struct net_device *dev, } #endif +static inline struct netdev_rx_queue * +netif_get_rx_queue(struct net_device *dev, unsigned int rxq) +{ + return dev->_rx + rxq; +} + #ifdef CONFIG_SYSFS static inline unsigned int get_netdev_rx_queue_index( struct netdev_rx_queue *queue) diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index a86e668..47cd0b7 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -151,6 +151,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, { u32 size = kattr->test.data_size_in; u32 repeat = kattr->test.repeat; + struct netdev_rx_queue *rxqueue; struct xdp_buff xdp = {}; u32 retval, duration; void *data; @@ -165,6 +166,9 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, xdp.data_meta = xdp.data; xdp.data_end = xdp.data + size; + rxqueue = netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); + xdp.rxq = &rxqueue->xdp_rxq; + retval = bpf_test_run(prog, &xdp, repeat, &duration); if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN) size = xdp.data_end - xdp.data; diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 697bd83..c0f16e9 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -7780,6 +7780,20 @@ static struct bpf_test tests[] = { .result = REJECT, }, { + "XDP, using ifindex from netdev", + .insns = { + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, + offsetof(struct xdp_md, ingress_ifindex)), + BPF_JMP_IMM(BPF_JLT, BPF_REG_2, 1, 1), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .result = ACCEPT, + .prog_type = BPF_PROG_TYPE_XDP, + .retval = 1, + }, + { "meta access, test1", .insns = { BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
syzkaller was able to generate the following XDP program ... (18) r0 = 0x0 (61) r5 = *(u32 *)(r1 +12) (04) (u32) r0 += (u32) 0 (95) exit ... and trigger a NULL pointer dereference in ___bpf_prog_run() via bpf_prog_test_run_xdp() where this was attempted to run. Reason is that recent xdp_rxq_info addition to XDP programs updated all drivers, but not bpf_prog_test_run_xdp(), where xdp_buff is set up. Thus when context rewriter does the deref on the netdev it's NULL at runtime. Fix it by using xdp_rxq from loopback dev. netif_get_rx_queue() helper can also be reused in various other locations later on. Fixes: 02dd3291b2f0 ("bpf: finally expose xdp_rxq_info to XDP bpf-programs") Reported-by: syzbot+1eb094057b338eb1fc00@syzkaller.appspotmail.com Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Jesper Dangaard Brouer <brouer@redhat.com> --- [ Note: Needs to wait till Linus pulled Dave's net-next so the affected commit lands in bpf tree. ] include/linux/netdevice.h | 6 ++++++ net/bpf/test_run.c | 4 ++++ tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ 3 files changed, 24 insertions(+)