* [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
@ 2024-04-01 13:34 Ziyang Xuan
2024-04-02 10:56 ` Florian Westphal
0 siblings, 1 reply; 7+ messages in thread
From: Ziyang Xuan @ 2024-04-01 13:34 UTC (permalink / raw)
To: pablo, kadlec, netfilter-devel
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(-)
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-01 13:34 [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get() Ziyang Xuan
@ 2024-04-02 10:56 ` Florian Westphal
2024-04-02 11:30 ` Pablo Neira Ayuso
2024-04-02 12:52 ` Ziyang Xuan (William)
0 siblings, 2 replies; 7+ messages in thread
From: Florian Westphal @ 2024-04-02 10:56 UTC (permalink / raw)
To: Ziyang Xuan; +Cc: pablo, kadlec, netfilter-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-02 10:56 ` Florian Westphal
@ 2024-04-02 11:30 ` Pablo Neira Ayuso
2024-04-02 12:58 ` Ziyang Xuan (William)
2024-04-02 12:52 ` Ziyang Xuan (William)
1 sibling, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2024-04-02 11:30 UTC (permalink / raw)
To: Florian Westphal; +Cc: Ziyang Xuan, kadlec, netfilter-devel
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")
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-02 10:56 ` Florian Westphal
2024-04-02 11:30 ` Pablo Neira Ayuso
@ 2024-04-02 12:52 ` Ziyang Xuan (William)
2024-04-02 13:55 ` Florian Westphal
1 sibling, 1 reply; 7+ messages in thread
From: Ziyang Xuan (William) @ 2024-04-02 12:52 UTC (permalink / raw)
To: Florian Westphal; +Cc: pablo, kadlec, netfilter-devel
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-02 11:30 ` Pablo Neira Ayuso
@ 2024-04-02 12:58 ` Ziyang Xuan (William)
0 siblings, 0 replies; 7+ messages in thread
From: Ziyang Xuan (William) @ 2024-04-02 12:58 UTC (permalink / raw)
To: Pablo Neira Ayuso, Florian Westphal; +Cc: kadlec, netfilter-devel
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-02 12:52 ` Ziyang Xuan (William)
@ 2024-04-02 13:55 ` Florian Westphal
2024-04-03 0:51 ` Ziyang Xuan (William)
0 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2024-04-02 13:55 UTC (permalink / raw)
To: Ziyang Xuan (William); +Cc: Florian Westphal, pablo, kadlec, netfilter-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()
2024-04-02 13:55 ` Florian Westphal
@ 2024-04-03 0:51 ` Ziyang Xuan (William)
0 siblings, 0 replies; 7+ messages in thread
From: Ziyang Xuan (William) @ 2024-04-03 0:51 UTC (permalink / raw)
To: Florian Westphal; +Cc: pablo, kadlec, netfilter-devel
> 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.
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-03 0:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-01 13:34 [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get() Ziyang Xuan
2024-04-02 10:56 ` Florian Westphal
2024-04-02 11:30 ` Pablo Neira Ayuso
2024-04-02 12:58 ` Ziyang Xuan (William)
2024-04-02 12:52 ` Ziyang Xuan (William)
2024-04-02 13:55 ` Florian Westphal
2024-04-03 0:51 ` Ziyang Xuan (William)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.