Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference
Eric Dumazet writes:
> On Wed, 2016-09-28 at 10:56 -0400, Aaron Conole wrote:
>> Eric Dumazet writes:
>>
>> > On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote:
>> >> It's possible for nf_hook_entry_head to return NULL. If two
>> >> nf_unregister_net_hook calls happen simultaneously with a single hook
>> >> entry in the list, both will enter the nf_hook_mutex critical section.
>> >> The first will successfully delete the head, but the second will see
>> >> this NULL pointer and attempt to dereference.
>> >>
>> >> This fix ensures that no null pointer dereference could occur when such
>> >> a condition happens.
>> >>
>> >> Signed-off-by: Aaron Conole
>> >> ---
>> >> net/netfilter/core.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
>> >> index 360c63d..e58e420 100644
>> >> --- a/net/netfilter/core.c
>> >> +++ b/net/netfilter/core.c
>> >> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const
>> >> struct nf_hook_ops *reg)
>> >>
>> >> mutex_lock(&nf_hook_mutex);
>> >> hooks_entry = nf_hook_entry_head(net, reg);
>> >> - if (hooks_entry->orig_ops == reg) {
>> >> + if (hooks_entry && hooks_entry->orig_ops == reg) {
>> >> nf_set_hooks_head(net, reg,
>> >> nf_entry_dereference(hooks_entry->next));
>> >> goto unlock;
>> >
>> > When was the bug added exactly ?
>>
>> Sunday, on the nf-next tree.
>>
>> > For all bug fixes, you need to add a Fixes: tag.
>> >
>> > Like :
>> >
>> > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked
>> > list")
>>
>> I would but it's in nf-next tree, and I'm not sure how pulls go. If
>> they are done via patch imports, then the sha sums will be wrong and the
>> commit message will be misleading. If the sums are preserved, then I
>> can resubmit with this information.
>>
>
> I gave the (12 digits) sha-1 as present in David Miller net-next tree.
>
> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d
>
>
> This wont change, because David never rebases his tree under normal
> operations.
>
> Thanks.
Thank you very much, Eric. I've reposted.
-Aaron
Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference
On Wed, 2016-09-28 at 10:56 -0400, Aaron Conole wrote:
> Eric Dumazet writes:
>
> > On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote:
> >> It's possible for nf_hook_entry_head to return NULL. If two
> >> nf_unregister_net_hook calls happen simultaneously with a single hook
> >> entry in the list, both will enter the nf_hook_mutex critical section.
> >> The first will successfully delete the head, but the second will see
> >> this NULL pointer and attempt to dereference.
> >>
> >> This fix ensures that no null pointer dereference could occur when such
> >> a condition happens.
> >>
> >> Signed-off-by: Aaron Conole
> >> ---
> >> net/netfilter/core.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> >> index 360c63d..e58e420 100644
> >> --- a/net/netfilter/core.c
> >> +++ b/net/netfilter/core.c
> >> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const
> >> struct nf_hook_ops *reg)
> >>
> >>mutex_lock(&nf_hook_mutex);
> >>hooks_entry = nf_hook_entry_head(net, reg);
> >> - if (hooks_entry->orig_ops == reg) {
> >> + if (hooks_entry && hooks_entry->orig_ops == reg) {
> >>nf_set_hooks_head(net, reg,
> >> nf_entry_dereference(hooks_entry->next));
> >>goto unlock;
> >
> > When was the bug added exactly ?
>
> Sunday, on the nf-next tree.
>
> > For all bug fixes, you need to add a Fixes: tag.
> >
> > Like :
> >
> > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
>
> I would but it's in nf-next tree, and I'm not sure how pulls go. If
> they are done via patch imports, then the sha sums will be wrong and the
> commit message will be misleading. If the sums are preserved, then I
> can resubmit with this information.
>
I gave the (12 digits) sha-1 as present in David Miller net-next tree.
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d
This wont change, because David never rebases his tree under normal
operations.
Thanks.
Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference
Eric Dumazet writes:
> On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote:
>> It's possible for nf_hook_entry_head to return NULL. If two
>> nf_unregister_net_hook calls happen simultaneously with a single hook
>> entry in the list, both will enter the nf_hook_mutex critical section.
>> The first will successfully delete the head, but the second will see
>> this NULL pointer and attempt to dereference.
>>
>> This fix ensures that no null pointer dereference could occur when such
>> a condition happens.
>>
>> Signed-off-by: Aaron Conole
>> ---
>> net/netfilter/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
>> index 360c63d..e58e420 100644
>> --- a/net/netfilter/core.c
>> +++ b/net/netfilter/core.c
>> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const
>> struct nf_hook_ops *reg)
>>
>> mutex_lock(&nf_hook_mutex);
>> hooks_entry = nf_hook_entry_head(net, reg);
>> -if (hooks_entry->orig_ops == reg) {
>> +if (hooks_entry && hooks_entry->orig_ops == reg) {
>> nf_set_hooks_head(net, reg,
>>nf_entry_dereference(hooks_entry->next));
>> goto unlock;
>
> When was the bug added exactly ?
Sunday, on the nf-next tree.
> For all bug fixes, you need to add a Fixes: tag.
>
> Like :
>
> Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
I would but it's in nf-next tree, and I'm not sure how pulls go. If
they are done via patch imports, then the sha sums will be wrong and the
commit message will be misleading. If the sums are preserved, then I
can resubmit with this information.
> So that 100 different people in stable teams do not have to do the
> archeology themselves ...
>
> Thanks.
Thanks for the review, Eric.
Re: [PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference
On Wed, 2016-09-28 at 09:12 -0400, Aaron Conole wrote:
> It's possible for nf_hook_entry_head to return NULL. If two
> nf_unregister_net_hook calls happen simultaneously with a single hook
> entry in the list, both will enter the nf_hook_mutex critical section.
> The first will successfully delete the head, but the second will see
> this NULL pointer and attempt to dereference.
>
> This fix ensures that no null pointer dereference could occur when such
> a condition happens.
>
> Signed-off-by: Aaron Conole
> ---
> net/netfilter/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 360c63d..e58e420 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct
> nf_hook_ops *reg)
>
> mutex_lock(&nf_hook_mutex);
> hooks_entry = nf_hook_entry_head(net, reg);
> - if (hooks_entry->orig_ops == reg) {
> + if (hooks_entry && hooks_entry->orig_ops == reg) {
> nf_set_hooks_head(net, reg,
> nf_entry_dereference(hooks_entry->next));
> goto unlock;
When was the bug added exactly ?
For all bug fixes, you need to add a Fixes: tag.
Like :
Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
So that 100 different people in stable teams do not have to do the archeology
themselves ...
Thanks.
[PATCH nf-next v3 1/2] netfilter: Fix potential null pointer dereference
It's possible for nf_hook_entry_head to return NULL. If two
nf_unregister_net_hook calls happen simultaneously with a single hook
entry in the list, both will enter the nf_hook_mutex critical section.
The first will successfully delete the head, but the second will see
this NULL pointer and attempt to dereference.
This fix ensures that no null pointer dereference could occur when such
a condition happens.
Signed-off-by: Aaron Conole
---
net/netfilter/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 360c63d..e58e420 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct
nf_hook_ops *reg)
mutex_lock(&nf_hook_mutex);
hooks_entry = nf_hook_entry_head(net, reg);
- if (hooks_entry->orig_ops == reg) {
+ if (hooks_entry && hooks_entry->orig_ops == reg) {
nf_set_hooks_head(net, reg,
nf_entry_dereference(hooks_entry->next));
goto unlock;
--
2.7.4
