Ziyang Xuan (William) <william.xuanziyang@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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.
