| Message ID | 87381ne3rq.fsf@x220.int.ebiederm.org |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Fri, Jun 19, 2015 at 05:23:37PM -0500, Eric W. Biederman wrote: > > If someone sends packets from one of the netdevice ingress hooks to > the a userspace queue, and then userspace later accepts the packet, > the netfilter code can enter an infinite loop as the list head will > never be found. > > Pass in the saved list_head to avoid this. There is no userspace queueing for netdevice yet, so this can be route through nf-next. Thanks. > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > net/netfilter/nf_queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > index cd60d397fe05..8a8b2abc35ff 100644 > --- a/net/netfilter/nf_queue.c > +++ b/net/netfilter/nf_queue.c > @@ -213,7 +213,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) > > if (verdict == NF_ACCEPT) { > next_hook: > - verdict = nf_iterate(&nf_hooks[entry->state.pf][entry->state.hook], > + verdict = nf_iterate(entry->state.hook_list, > skb, &entry->state, &elem); > } > > -- > 2.2.1 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
Pablo Neira Ayuso <pablo@netfilter.org> writes: > On Fri, Jun 19, 2015 at 05:23:37PM -0500, Eric W. Biederman wrote: >> >> If someone sends packets from one of the netdevice ingress hooks to >> the a userspace queue, and then userspace later accepts the packet, >> the netfilter code can enter an infinite loop as the list head will >> never be found. >> >> Pass in the saved list_head to avoid this. > > There is no userspace queueing for netdevice yet, so this can be route > through nf-next. Thanks. *scratches head* the netdevice queueing is in the netfilter core. netfilter_ingress calls nf_hook_slow. The queuing happens in nf_hook_slow if anything returns the verdict queue it. This patch applies to Linus's tree. So how in the world does this not need to be ported to 4.1? >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> net/netfilter/nf_queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c >> index cd60d397fe05..8a8b2abc35ff 100644 >> --- a/net/netfilter/nf_queue.c >> +++ b/net/netfilter/nf_queue.c >> @@ -213,7 +213,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) >> >> if (verdict == NF_ACCEPT) { >> next_hook: >> - verdict = nf_iterate(&nf_hooks[entry->state.pf][entry->state.hook], >> + verdict = nf_iterate(entry->state.hook_list, >> skb, &entry->state, &elem); >> } >> >> -- >> 2.2.1 >> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
On Sat, Jun 20, 2015 at 09:08:20AM -0500, Eric W. Biederman wrote: > Pablo Neira Ayuso <pablo@netfilter.org> writes: > > > On Fri, Jun 19, 2015 at 05:23:37PM -0500, Eric W. Biederman wrote: > >> > >> If someone sends packets from one of the netdevice ingress hooks to > >> the a userspace queue, and then userspace later accepts the packet, > >> the netfilter code can enter an infinite loop as the list head will > >> never be found. > >> > >> Pass in the saved list_head to avoid this. > > > > There is no userspace queueing for netdevice yet, so this can be route > > through nf-next. Thanks. > > *scratches head* the netdevice queueing is in the netfilter core. > > netfilter_ingress calls nf_hook_slow. The queuing happens in > nf_hook_slow if anything returns the verdict queue it. > > This patch applies to Linus's tree. > > So how in the world does this not need to be ported to 4.1? There is no nfnetlink_queue support for the netdev family at this moment, so this can't be triggered unless you use an out of tree module. I have a patch here to add a static key to disable userspace queueing per family using a static key so that part would be basically inactive. But if you really want to see this in 4.1, no problem, please just let me know and I'll pass it to David, as I said it's basically not resolving any urgent problem so this is not harming. Thank you. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
Pablo Neira Ayuso <pablo@netfilter.org> writes: > On Sat, Jun 20, 2015 at 09:08:20AM -0500, Eric W. Biederman wrote: >> Pablo Neira Ayuso <pablo@netfilter.org> writes: >> >> > On Fri, Jun 19, 2015 at 05:23:37PM -0500, Eric W. Biederman wrote: >> >> >> >> If someone sends packets from one of the netdevice ingress hooks to >> >> the a userspace queue, and then userspace later accepts the packet, >> >> the netfilter code can enter an infinite loop as the list head will >> >> never be found. >> >> >> >> Pass in the saved list_head to avoid this. >> > >> > There is no userspace queueing for netdevice yet, so this can be route >> > through nf-next. Thanks. >> >> *scratches head* the netdevice queueing is in the netfilter core. >> >> netfilter_ingress calls nf_hook_slow. The queuing happens in >> nf_hook_slow if anything returns the verdict queue it. >> >> This patch applies to Linus's tree. >> >> So how in the world does this not need to be ported to 4.1? > > There is no nfnetlink_queue support for the netdev family at this > moment, so this can't be triggered unless you use an out of tree > module. > > I have a patch here to add a static key to disable userspace queueing > per family using a static key so that part would be basically > inactive. > > But if you really want to see this in 4.1, no problem, please just let > me know and I'll pass it to David, as I said it's basically not > resolving any urgent problem so this is not harming. So I have read through the code and I simply do not see anywhere that would prevent code in nft_queue to be called on a per netdevice chain, or where in nfqnl_enqueue_packet we would decided not to queue the packet. Could you please point me to what in the code makes this code path impossible to use? Right now it looks like something that is triggerable in 4.1. Eric -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
On Mon, Jun 22, 2015 at 09:56:37AM -0500, Eric W. Biederman wrote: > Pablo Neira Ayuso <pablo@netfilter.org> writes: [...] > > There is no nfnetlink_queue support for the netdev family at this > > moment, so this can't be triggered unless you use an out of tree > > module. > > > > I have a patch here to add a static key to disable userspace queueing > > per family using a static key so that part would be basically > > inactive. > > > > But if you really want to see this in 4.1, no problem, please just let > > me know and I'll pass it to David, as I said it's basically not > > resolving any urgent problem so this is not harming. > > So I have read through the code and I simply do not see anywhere > that would prevent code in nft_queue to be called on a per netdevice > chain, or where in nfqnl_enqueue_packet we would decided not to queue > the packet. > > Could you please point me to what in the code makes this code path > impossible to use? int nf_queue(struct sk_buff *skb, struct nf_hook_ops *elem, struct nf_hook_state *state, unsigned int queuenum) { const struct nf_afinfo *afinfo; ... afinfo = nf_get_afinfo(state->pf); if (!afinfo) goto err_unlock; ... } There is no afinfo for NFPROTO_NETDEV, so the packet the packet will be dropped. Therefore, there is no way nf_reinject() can be called neither for bridge, arp, inet and netdev at this moment. I have a patch here to restrict this easily in nft_queue so the user knows this can't be used at configuration time. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> writes: > On Mon, Jun 22, 2015 at 09:56:37AM -0500, Eric W. Biederman wrote: >> Pablo Neira Ayuso <pablo@netfilter.org> writes: > [...] >> > There is no nfnetlink_queue support for the netdev family at this >> > moment, so this can't be triggered unless you use an out of tree >> > module. >> > >> > I have a patch here to add a static key to disable userspace queueing >> > per family using a static key so that part would be basically >> > inactive. >> > >> > But if you really want to see this in 4.1, no problem, please just let >> > me know and I'll pass it to David, as I said it's basically not >> > resolving any urgent problem so this is not harming. >> >> So I have read through the code and I simply do not see anywhere >> that would prevent code in nft_queue to be called on a per netdevice >> chain, or where in nfqnl_enqueue_packet we would decided not to queue >> the packet. >> >> Could you please point me to what in the code makes this code path >> impossible to use? > > int nf_queue(struct sk_buff *skb, > struct nf_hook_ops *elem, > struct nf_hook_state *state, > unsigned int queuenum) > { > const struct nf_afinfo *afinfo; > ... > > afinfo = nf_get_afinfo(state->pf); > if (!afinfo) > goto err_unlock; > > ... > } > > There is no afinfo for NFPROTO_NETDEV, so the packet the packet will > be dropped. > > Therefore, there is no way nf_reinject() can be called neither for > bridge, arp, inet and netdev at this moment. > > I have a patch here to restrict this easily in nft_queue so the user > knows this can't be used at configuration time. Subtle but it I see it now. Thank you. With only nf_register_afinfo calls for ipv4 and ipv6 present in the kernel it is clear that we can not run into trouble in the nedev ingress path, and so my fix clearly does not need to be backported. Eric -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c index cd60d397fe05..8a8b2abc35ff 100644 --- a/net/netfilter/nf_queue.c +++ b/net/netfilter/nf_queue.c @@ -213,7 +213,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) if (verdict == NF_ACCEPT) { next_hook: - verdict = nf_iterate(&nf_hooks[entry->state.pf][entry->state.hook], + verdict = nf_iterate(entry->state.hook_list, skb, &entry->state, &elem); }
If someone sends packets from one of the netdevice ingress hooks to the a userspace queue, and then userspace later accepts the packet, the netfilter code can enter an infinite loop as the list head will never be found. Pass in the saved list_head to avoid this. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- net/netfilter/nf_queue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)