| Message ID | 1424448670-4458-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On 20.02, Pablo Neira Ayuso wrote: > We have several problems in this path: > > 1) There is a use-after-free when removing individual elements from > the commit path. > > 2) We have to uninit() the data part of the element from the abort > path to avoid a chain refcount leak. > > 3) We have to check for set->flags to see if there's a mapping, instead > of the element flags. > > 4) We have to check for !(flags & NFT_SET_ELEM_INTERVAL_END) to skip > elements that are part of the interval that have no data part, so > they don't need to be uninit(). Just wondering, in the delete case, don't we need to set the flags in the sets' ->get() function for this to work? -- 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
On Sat, Feb 21, 2015 at 10:39:18AM +0000, Patrick McHardy wrote: > On 20.02, Pablo Neira Ayuso wrote: > > We have several problems in this path: > > > > 1) There is a use-after-free when removing individual elements from > > the commit path. > > > > 2) We have to uninit() the data part of the element from the abort > > path to avoid a chain refcount leak. > > > > 3) We have to check for set->flags to see if there's a mapping, instead > > of the element flags. > > > > 4) We have to check for !(flags & NFT_SET_ELEM_INTERVAL_END) to skip > > elements that are part of the interval that have no data part, so > > they don't need to be uninit(). > > Just wondering, in the delete case, don't we need to set the flags in > the sets' ->get() function for this to work? They are already set from hash and rbtree, so we only need to add the check for NFT_SET_ELEM_INTERVAL_END from the commit path in nf_tables_api.c Unless you have any further concern, I'll pass up this soon. -- 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
On 24.02, Pablo Neira Ayuso wrote: > On Sat, Feb 21, 2015 at 10:39:18AM +0000, Patrick McHardy wrote: > > On 20.02, Pablo Neira Ayuso wrote: > > > We have several problems in this path: > > > > > > 1) There is a use-after-free when removing individual elements from > > > the commit path. > > > > > > 2) We have to uninit() the data part of the element from the abort > > > path to avoid a chain refcount leak. > > > > > > 3) We have to check for set->flags to see if there's a mapping, instead > > > of the element flags. > > > > > > 4) We have to check for !(flags & NFT_SET_ELEM_INTERVAL_END) to skip > > > elements that are part of the interval that have no data part, so > > > they don't need to be uninit(). > > > > Just wondering, in the delete case, don't we need to set the flags in > > the sets' ->get() function for this to work? > > They are already set from hash and rbtree, so we only need to add the > check for NFT_SET_ELEM_INTERVAL_END from the commit path in nf_tables_api.c > > Unless you have any further concern, I'll pass up this soon. Right, in the hash case it's 0 anyways and rbtree does set them. Looks good to me. -- 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
On Tue, Feb 24, 2015 at 02:39:33PM +0000, Patrick McHardy wrote: > On 24.02, Pablo Neira Ayuso wrote: > > On Sat, Feb 21, 2015 at 10:39:18AM +0000, Patrick McHardy wrote: > > > On 20.02, Pablo Neira Ayuso wrote: > > > > We have several problems in this path: > > > > > > > > 1) There is a use-after-free when removing individual elements from > > > > the commit path. > > > > > > > > 2) We have to uninit() the data part of the element from the abort > > > > path to avoid a chain refcount leak. > > > > > > > > 3) We have to check for set->flags to see if there's a mapping, instead > > > > of the element flags. > > > > > > > > 4) We have to check for !(flags & NFT_SET_ELEM_INTERVAL_END) to skip > > > > elements that are part of the interval that have no data part, so > > > > they don't need to be uninit(). > > > > > > Just wondering, in the delete case, don't we need to set the flags in > > > the sets' ->get() function for this to work? > > > > They are already set from hash and rbtree, so we only need to add the > > check for NFT_SET_ELEM_INTERVAL_END from the commit path in nf_tables_api.c > > > > Unless you have any further concern, I'll pass up this soon. > > Right, in the hash case it's 0 anyways and rbtree does set them. > > Looks good to me. Ok, I'll pass this to David, thanks. -- 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_tables_api.c b/net/netfilter/nf_tables_api.c index 199fd0f..a8c9462 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3612,12 +3612,11 @@ static int nf_tables_commit(struct sk_buff *skb) &te->elem, NFT_MSG_DELSETELEM, 0); te->set->ops->get(te->set, &te->elem); - te->set->ops->remove(te->set, &te->elem); nft_data_uninit(&te->elem.key, NFT_DATA_VALUE); - if (te->elem.flags & NFT_SET_MAP) { - nft_data_uninit(&te->elem.data, - te->set->dtype); - } + if (te->set->flags & NFT_SET_MAP && + !(te->elem.flags & NFT_SET_ELEM_INTERVAL_END)) + nft_data_uninit(&te->elem.data, te->set->dtype); + te->set->ops->remove(te->set, &te->elem); nft_trans_destroy(trans); break; } @@ -3658,7 +3657,7 @@ static int nf_tables_abort(struct sk_buff *skb) { struct net *net = sock_net(skb->sk); struct nft_trans *trans, *next; - struct nft_set *set; + struct nft_trans_elem *te; list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) { switch (trans->msg_type) { @@ -3719,9 +3718,13 @@ static int nf_tables_abort(struct sk_buff *skb) break; case NFT_MSG_NEWSETELEM: nft_trans_elem_set(trans)->nelems--; - set = nft_trans_elem_set(trans); - set->ops->get(set, &nft_trans_elem(trans)); - set->ops->remove(set, &nft_trans_elem(trans)); + te = (struct nft_trans_elem *)trans->data; + te->set->ops->get(te->set, &te->elem); + nft_data_uninit(&te->elem.key, NFT_DATA_VALUE); + if (te->set->flags & NFT_SET_MAP && + !(te->elem.flags & NFT_SET_ELEM_INTERVAL_END)) + nft_data_uninit(&te->elem.data, te->set->dtype); + te->set->ops->remove(te->set, &te->elem); nft_trans_destroy(trans); break; case NFT_MSG_DELSETELEM:
We have several problems in this path: 1) There is a use-after-free when removing individual elements from the commit path. 2) We have to uninit() the data part of the element from the abort path to avoid a chain refcount leak. 3) We have to check for set->flags to see if there's a mapping, instead of the element flags. 4) We have to check for !(flags & NFT_SET_ELEM_INTERVAL_END) to skip elements that are part of the interval that have no data part, so they don't need to be uninit(). Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- This fix replaces the following two patches: http://patchwork.ozlabs.org/patch/440781/ http://patchwork.ozlabs.org/patch/440782/ Thanks to Patrick for helping on discussing this fix! net/netfilter/nf_tables_api.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)