| Message ID | 20240401133455.1945938-1-william.xuanziyang@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get() | expand |
Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can > concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). > And thhere is not any protection when iterate over nf_tables_flowtables > list in __nft_flowtable_type_get(). Therefore, there is pertential > data-race of nf_tables_flowtables list entry. > > Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over > nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. I don't think this resolves the described race. > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > net/netfilter/nf_tables_api.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index fd86f2720c9e..fbf38e32f11d 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) > { > const struct nf_flowtable_type *type; > > - list_for_each_entry(type, &nf_tables_flowtables, list) { > - if (family == type->family) > + rcu_read_lock() > + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { > + if (family == type->family) { > + rcu_read_unlock(); > return type; This means 'type' can be non-null while module is being unloaded, before refcount increment. You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, and release it after refcount on module owner failed or succeeded.
On Tue, Apr 02, 2024 at 12:56:42PM +0200, Florian Westphal wrote: > Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can > > concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). > > And thhere is not any protection when iterate over nf_tables_flowtables > > list in __nft_flowtable_type_get(). Therefore, there is pertential > > data-race of nf_tables_flowtables list entry. > > > > Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over > > nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. > > I don't think this resolves the described race. > > > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > --- > > net/netfilter/nf_tables_api.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index fd86f2720c9e..fbf38e32f11d 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) > > { > > const struct nf_flowtable_type *type; > > > > - list_for_each_entry(type, &nf_tables_flowtables, list) { > > - if (family == type->family) > > + rcu_read_lock() > > + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { > > + if (family == type->family) { > > + rcu_read_unlock(); > > return type; > > This means 'type' can be non-null while module is being unloaded, > before refcount increment. > > You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, > and release it after refcount on module owner failed or succeeded. And these need to be rcu protected: static LIST_HEAD(nf_tables_expressions); static LIST_HEAD(nf_tables_objects); static LIST_HEAD(nf_tables_flowtables); for a complete fix for: f102d66b335a ("netfilter: nf_tables: use dedicated mutex to guard transactions")
> Ziyang Xuan <william.xuanziyang@huawei.com> wrote: >> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can >> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). >> And thhere is not any protection when iterate over nf_tables_flowtables >> list in __nft_flowtable_type_get(). Therefore, there is pertential >> data-race of nf_tables_flowtables list entry. >> >> Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over >> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. > > I don't think this resolves the described race. > >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> net/netfilter/nf_tables_api.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c >> index fd86f2720c9e..fbf38e32f11d 100644 >> --- a/net/netfilter/nf_tables_api.c >> +++ b/net/netfilter/nf_tables_api.c >> @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) >> { >> const struct nf_flowtable_type *type; >> >> - list_for_each_entry(type, &nf_tables_flowtables, list) { >> - if (family == type->family) >> + rcu_read_lock() >> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { >> + if (family == type->family) { >> + rcu_read_unlock(); >> return type; > > This means 'type' can be non-null while module is being unloaded, > before refcount increment. > > You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, > and release it after refcount on module owner failed or succeeded. > . In fact, I just want to resolve the potential tear-down problem about list entry here. As following: void nft_unregister_flowtable_type(struct nf_flowtable_type *type) { nfnl_lock(NFNL_SUBSYS_NFTABLES); /* just use WRITE_ONCE() inside, but not rcu_assign_pointer(). * It can not form competition protection with rcu_read_lock(). */ list_del_rcu(&type->list); nfnl_unlock(NFNL_SUBSYS_NFTABLES); } static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) { const struct nf_flowtable_type *type; /* Get list entry use READ_ONCE() inside. And I think rcu_read_lock() maybe unnecessary. */ list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { if (family == type->family) return type; } return NULL; } static const struct nf_flowtable_type * nft_flowtable_type_get(struct net *net, u8 family) { const struct nf_flowtable_type *type; type = __nft_flowtable_type_get(family); /* If type is non-NULL, try to get module. If the module * state is not ok, it will fail here. */ if (type != NULL && try_module_get(type->owner)) return type; lockdep_nfnl_nft_mutex_not_held(); #ifdef CONFIG_MODULES if (type == NULL) { if (nft_request_module(net, "nf-flowtable-%u", family) == -EAGAIN) return ERR_PTR(-EAGAIN); } #endif return ERR_PTR(-ENOENT); } So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now. William Xuan Best regards.
> On Tue, Apr 02, 2024 at 12:56:42PM +0200, Florian Westphal wrote: >> Ziyang Xuan <william.xuanziyang@huawei.com> wrote: >>> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can >>> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). >>> And thhere is not any protection when iterate over nf_tables_flowtables >>> list in __nft_flowtable_type_get(). Therefore, there is pertential >>> data-race of nf_tables_flowtables list entry. >>> >>> Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over >>> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. >> >> I don't think this resolves the described race. >> >>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >>> --- >>> net/netfilter/nf_tables_api.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c >>> index fd86f2720c9e..fbf38e32f11d 100644 >>> --- a/net/netfilter/nf_tables_api.c >>> +++ b/net/netfilter/nf_tables_api.c >>> @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) >>> { >>> const struct nf_flowtable_type *type; >>> >>> - list_for_each_entry(type, &nf_tables_flowtables, list) { >>> - if (family == type->family) >>> + rcu_read_lock() >>> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { >>> + if (family == type->family) { >>> + rcu_read_unlock(); >>> return type; >> >> This means 'type' can be non-null while module is being unloaded, >> before refcount increment. >> >> You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, >> and release it after refcount on module owner failed or succeeded. > > And these need to be rcu protected: > > static LIST_HEAD(nf_tables_expressions); > static LIST_HEAD(nf_tables_objects); > static LIST_HEAD(nf_tables_flowtables); > > for a complete fix for: > > f102d66b335a ("netfilter: nf_tables: use dedicated mutex to guard transactions") > . > I would be happy to do these. Thank you for your kind remind! William Xuan Best regards.
Ziyang Xuan (William) <william.xuanziyang@huawei.com> wrote: > >> Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over > >> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. > > > > I don't think this resolves the described race. > > > >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > >> --- > >> net/netfilter/nf_tables_api.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > >> index fd86f2720c9e..fbf38e32f11d 100644 > >> --- a/net/netfilter/nf_tables_api.c > >> +++ b/net/netfilter/nf_tables_api.c > >> @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) > >> { > >> const struct nf_flowtable_type *type; > >> > >> - list_for_each_entry(type, &nf_tables_flowtables, list) { > >> - if (family == type->family) > >> + rcu_read_lock() > >> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { > >> + if (family == type->family) { > >> + rcu_read_unlock(); > >> return type; > > > > This means 'type' can be non-null while module is being unloaded, > > before refcount increment. > > > > You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, > > and release it after refcount on module owner failed or succeeded. > > . > In fact, I just want to resolve the potential tear-down problem about list entry here. cpu1 cpu2 rmmod flowtable_type nft_flowtable_type_get __nft_flowtable_type_get finds family == type->family list_del_rcu(type) CPU INTERRUPTED rmmod completes nft_flowtable_type_get calls if (type != NULL && try_module_get(type->owner)) ---> UaF Skeleton fix: nft_flowtable_type_get(struct net *net, u8 family) { const struct nf_flowtable_type *type; + rcu_read_lock(); type = __nft_flowtable_type_get(family); .... if (type != NULL && try_module_get(type->owner)) { rcu_read_unlock(); return type; } rcu_read_unlock(); This avoids the above UaF, rmmod cannot complete fully until after rcu read lock section is done. (There is a synchronize_rcu in module teardown path before the data section is freed). > So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now. I don't think so.
> Ziyang Xuan (William) <william.xuanziyang@huawei.com> wrote: >>>> Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over >>>> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. >>> >>> I don't think this resolves the described race. >>> >>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >>>> --- >>>> net/netfilter/nf_tables_api.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c >>>> index fd86f2720c9e..fbf38e32f11d 100644 >>>> --- a/net/netfilter/nf_tables_api.c >>>> +++ b/net/netfilter/nf_tables_api.c >>>> @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) >>>> { >>>> const struct nf_flowtable_type *type; >>>> >>>> - list_for_each_entry(type, &nf_tables_flowtables, list) { >>>> - if (family == type->family) >>>> + rcu_read_lock() >>>> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { >>>> + if (family == type->family) { >>>> + rcu_read_unlock(); >>>> return type; >>> >>> This means 'type' can be non-null while module is being unloaded, >>> before refcount increment. >>> >>> You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get, >>> and release it after refcount on module owner failed or succeeded. >>> . >> In fact, I just want to resolve the potential tear-down problem about list entry here. > > cpu1 cpu2 > rmmod > flowtable_type > > nft_flowtable_type_get > __nft_flowtable_type_get > finds family == type->family > list_del_rcu(type) > > CPU INTERRUPTED > rmmod completes > > nft_flowtable_type_get calls > if (type != NULL && try_module_get(type->owner)) > ---> UaF > > Skeleton fix: > > nft_flowtable_type_get(struct net *net, u8 family) > { > const struct nf_flowtable_type *type; > > + rcu_read_lock(); > type = __nft_flowtable_type_get(family); > .... > if (type != NULL && try_module_get(type->owner)) { > rcu_read_unlock(); > return type; > } > > rcu_read_unlock(); > > This avoids the above UaF, rmmod cannot complete fully until after > rcu read lock section is done. (There is a synchronize_rcu in module > teardown path before the data section is freed). I see. Thank you for your patient analysis! >> So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now. > > I don't think so. > . >
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index fd86f2720c9e..fbf38e32f11d 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) { const struct nf_flowtable_type *type; - list_for_each_entry(type, &nf_tables_flowtables, list) { - if (family == type->family) + rcu_read_lock() + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { + if (family == type->family) { + rcu_read_unlock(); return type; + } } + rcu_read_unlock(); return NULL; }
nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). And thhere is not any protection when iterate over nf_tables_flowtables list in __nft_flowtable_type_get(). Therefore, there is pertential data-race of nf_tables_flowtables list entry. Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it. Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/netfilter/nf_tables_api.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)