linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free
@ 2021-08-26  3:42 王贇
  2021-08-27  0:09 ` Paul Moore
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: 王贇 @ 2021-08-26  3:42 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
failed, we sometime observe panic:

  BUG: kernel NULL pointer dereference, address:
  ...
  RIP: 0010:cipso_v4_doi_free+0x3a/0x80
  ...
  Call Trace:
   netlbl_cipsov4_add_std+0xf4/0x8c0
   netlbl_cipsov4_add+0x13f/0x1b0
   genl_family_rcv_msg_doit.isra.15+0x132/0x170
   genl_rcv_msg+0x125/0x240

This is because in cipso_v4_doi_free() there is no check
on 'doi_def->map.std' when 'doi_def->type' equal 1, which
is possibe, since netlbl_cipsov4_add_std() haven't initialize
it before alloc 'doi_def->map.std'.

This patch just add the check to prevent panic happen for similar
cases.

Reported-by: Abaci <abaci@linux.alibaba.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---

 net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 099259f..7fbd0b5 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
 	if (!doi_def)
 		return;

-	switch (doi_def->type) {
-	case CIPSO_V4_MAP_TRANS:
-		kfree(doi_def->map.std->lvl.cipso);
-		kfree(doi_def->map.std->lvl.local);
-		kfree(doi_def->map.std->cat.cipso);
-		kfree(doi_def->map.std->cat.local);
-		kfree(doi_def->map.std);
-		break;
+	if (doi_def->map.std) {
+		switch (doi_def->type) {
+		case CIPSO_V4_MAP_TRANS:
+			kfree(doi_def->map.std->lvl.cipso);
+			kfree(doi_def->map.std->lvl.local);
+			kfree(doi_def->map.std->cat.cipso);
+			kfree(doi_def->map.std->cat.local);
+			kfree(doi_def->map.std);
+			break;
+		}
 	}
 	kfree(doi_def);
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-26  3:42 [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free 王贇
@ 2021-08-27  0:09 ` Paul Moore
  2021-08-30 10:20   ` 王贇
  2021-08-30 10:14 ` 王贇
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-08-27  0:09 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Wed, Aug 25, 2021 at 11:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
>   BUG: kernel NULL pointer dereference, address:
>   ...
>   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
>   ...
>   Call Trace:
>    netlbl_cipsov4_add_std+0xf4/0x8c0
>    netlbl_cipsov4_add+0x13f/0x1b0
>    genl_family_rcv_msg_doit.isra.15+0x132/0x170
>    genl_rcv_msg+0x125/0x240
>
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when 'doi_def->type' equal 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
>
> This patch just add the check to prevent panic happen for similar
> cases.
>
> Reported-by: Abaci <abaci@linux.alibaba.com>
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> ---
>
>  net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

Thanks for the problem report.  It's hard to say for certain due to
the abbreviated backtrace without line number information, but it
looks like the problem you are describing is happening when the
allocation for doi_def->map.std fails near the top of
netlbl_cipsov4_add_std() which causes the function to jump the
add_std_failure target which ends up calling cipso_v4_doi_free().

  doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
  if (doi_def == NULL)
    return -ENOMEM;
  doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
  if (doi_def->map.std == NULL) {
    ret_val = -ENOMEM;
    goto add_std_failure;
  }
  ...
  add_std_failure:
    cipso_v4_doi_free(doi_def);

Since the doi_def allocation is not zero'd out, it is possible that
the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
the doi_def->map.std allocation fails, causing the NULL pointer deref
in cipso_v4_doi_free().  As this is the only case where we would see a
problem like this, I suggest a better solution would be to change the
if-block following the doi_def->map.std allocation to something like
this:

  doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
  if (doi_def->map.std == NULL) {
    kfree(doi_def);
    return -ENOMEM;
  }

> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 099259f..7fbd0b5 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>         if (!doi_def)
>                 return;
>
> -       switch (doi_def->type) {
> -       case CIPSO_V4_MAP_TRANS:
> -               kfree(doi_def->map.std->lvl.cipso);
> -               kfree(doi_def->map.std->lvl.local);
> -               kfree(doi_def->map.std->cat.cipso);
> -               kfree(doi_def->map.std->cat.local);
> -               kfree(doi_def->map.std);
> -               break;
> +       if (doi_def->map.std) {
> +               switch (doi_def->type) {
> +               case CIPSO_V4_MAP_TRANS:
> +                       kfree(doi_def->map.std->lvl.cipso);
> +                       kfree(doi_def->map.std->lvl.local);
> +                       kfree(doi_def->map.std->cat.cipso);
> +                       kfree(doi_def->map.std->cat.local);
> +                       kfree(doi_def->map.std);
> +                       break;
> +               }
>         }
>         kfree(doi_def);
>  }
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-26  3:42 [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free 王贇
  2021-08-27  0:09 ` Paul Moore
@ 2021-08-30 10:14 ` 王贇
  2021-08-30 10:28 ` [PATCH v2] " 王贇
  2021-09-03  2:27 ` [PATCH] net: remove the unnecessary check in cipso_v4_doi_free 王贇
  3 siblings, 0 replies; 25+ messages in thread
From: 王贇 @ 2021-08-30 10:14 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

Just a ping... Should we fix this?

Regards,
Michael Wang

On 2021/8/26 上午11:42, 王贇 wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
> 
>   BUG: kernel NULL pointer dereference, address:
>   ...
>   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
>   ...
>   Call Trace:
>    netlbl_cipsov4_add_std+0xf4/0x8c0
>    netlbl_cipsov4_add+0x13f/0x1b0
>    genl_family_rcv_msg_doit.isra.15+0x132/0x170
>    genl_rcv_msg+0x125/0x240
> 
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when 'doi_def->type' equal 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
> 
> This patch just add the check to prevent panic happen for similar
> cases.
> 
> Reported-by: Abaci <abaci@linux.alibaba.com>
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> ---
> 
>  net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 099259f..7fbd0b5 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>  	if (!doi_def)
>  		return;
> 
> -	switch (doi_def->type) {
> -	case CIPSO_V4_MAP_TRANS:
> -		kfree(doi_def->map.std->lvl.cipso);
> -		kfree(doi_def->map.std->lvl.local);
> -		kfree(doi_def->map.std->cat.cipso);
> -		kfree(doi_def->map.std->cat.local);
> -		kfree(doi_def->map.std);
> -		break;
> +	if (doi_def->map.std) {
> +		switch (doi_def->type) {
> +		case CIPSO_V4_MAP_TRANS:
> +			kfree(doi_def->map.std->lvl.cipso);
> +			kfree(doi_def->map.std->lvl.local);
> +			kfree(doi_def->map.std->cat.cipso);
> +			kfree(doi_def->map.std->cat.local);
> +			kfree(doi_def->map.std);
> +			break;
> +		}
>  	}
>  	kfree(doi_def);
>  }
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-27  0:09 ` Paul Moore
@ 2021-08-30 10:20   ` 王贇
  0 siblings, 0 replies; 25+ messages in thread
From: 王贇 @ 2021-08-30 10:20 UTC (permalink / raw)
  To: Paul Moore
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

Hi, Paul

I'm sorry for missing this mail since my stupid filter rules...

Will send a new one soon as you suggested :-)

Regards,
Michael Wang

On 2021/8/27 上午8:09, Paul Moore wrote:
[snip]
>>
>> Reported-by: Abaci <abaci@linux.alibaba.com>
>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>> ---
>>
>>  net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> Thanks for the problem report.  It's hard to say for certain due to
> the abbreviated backtrace without line number information, but it
> looks like the problem you are describing is happening when the
> allocation for doi_def->map.std fails near the top of
> netlbl_cipsov4_add_std() which causes the function to jump the
> add_std_failure target which ends up calling cipso_v4_doi_free().
> 
>   doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
>   if (doi_def == NULL)
>     return -ENOMEM;
>   doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
>   if (doi_def->map.std == NULL) {
>     ret_val = -ENOMEM;
>     goto add_std_failure;
>   }
>   ...
>   add_std_failure:
>     cipso_v4_doi_free(doi_def);
> 
> Since the doi_def allocation is not zero'd out, it is possible that
> the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
> the doi_def->map.std allocation fails, causing the NULL pointer deref
> in cipso_v4_doi_free().  As this is the only case where we would see a
> problem like this, I suggest a better solution would be to change the
> if-block following the doi_def->map.std allocation to something like
> this:
> 
>   doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
>   if (doi_def->map.std == NULL) {
>     kfree(doi_def);
>     return -ENOMEM;
>   }
> 
>> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
>> index 099259f..7fbd0b5 100644
>> --- a/net/ipv4/cipso_ipv4.c
>> +++ b/net/ipv4/cipso_ipv4.c
>> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>>         if (!doi_def)
>>                 return;
>>
>> -       switch (doi_def->type) {
>> -       case CIPSO_V4_MAP_TRANS:
>> -               kfree(doi_def->map.std->lvl.cipso);
>> -               kfree(doi_def->map.std->lvl.local);
>> -               kfree(doi_def->map.std->cat.cipso);
>> -               kfree(doi_def->map.std->cat.local);
>> -               kfree(doi_def->map.std);
>> -               break;
>> +       if (doi_def->map.std) {
>> +               switch (doi_def->type) {
>> +               case CIPSO_V4_MAP_TRANS:
>> +                       kfree(doi_def->map.std->lvl.cipso);
>> +                       kfree(doi_def->map.std->lvl.local);
>> +                       kfree(doi_def->map.std->cat.cipso);
>> +                       kfree(doi_def->map.std->cat.local);
>> +                       kfree(doi_def->map.std);
>> +                       break;
>> +               }
>>         }
>>         kfree(doi_def);
>>  }
>> --
>> 1.8.3.1
>>
> 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-26  3:42 [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free 王贇
  2021-08-27  0:09 ` Paul Moore
  2021-08-30 10:14 ` 王贇
@ 2021-08-30 10:28 ` 王贇
  2021-08-30 11:30   ` patchwork-bot+netdevbpf
                     ` (2 more replies)
  2021-09-03  2:27 ` [PATCH] net: remove the unnecessary check in cipso_v4_doi_free 王贇
  3 siblings, 3 replies; 25+ messages in thread
From: 王贇 @ 2021-08-30 10:28 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
failed, we sometime observe panic:

  BUG: kernel NULL pointer dereference, address:
  ...
  RIP: 0010:cipso_v4_doi_free+0x3a/0x80
  ...
  Call Trace:
   netlbl_cipsov4_add_std+0xf4/0x8c0
   netlbl_cipsov4_add+0x13f/0x1b0
   genl_family_rcv_msg_doit.isra.15+0x132/0x170
   genl_rcv_msg+0x125/0x240

This is because in cipso_v4_doi_free() there is no check
on 'doi_def->map.std' when doi_def->type got value 1, which
is possibe, since netlbl_cipsov4_add_std() haven't initialize
it before alloc 'doi_def->map.std'.

This patch just add the check to prevent panic happen in similar
cases.

Reported-by: Abaci <abaci@linux.alibaba.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 net/netlabel/netlabel_cipso_v4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index baf2357..344c228 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
 		return -ENOMEM;
 	doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
 	if (doi_def->map.std == NULL) {
-		ret_val = -ENOMEM;
-		goto add_std_failure;
+		kfree(doi_def);
+		return -ENOMEM;
 	}
 	doi_def->type = CIPSO_V4_MAP_TRANS;

-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-30 10:28 ` [PATCH v2] " 王贇
@ 2021-08-30 11:30   ` patchwork-bot+netdevbpf
  2021-08-30 14:17   ` Paul Moore
  2021-09-01  2:18   ` [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free" 王贇
  2 siblings, 0 replies; 25+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-30 11:30 UTC (permalink / raw)
  To: =?utf-8?b?546L6LSHIDx5dW4ud2FuZ0BsaW51eC5hbGliYWJhLmNvbT4=?=
  Cc: paul, davem, yoshfuji, dsahern, kuba, netdev,
	linux-security-module, linux-kernel

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 30 Aug 2021 18:28:01 +0800 you wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
> 
>   BUG: kernel NULL pointer dereference, address:
>   ...
>   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
>   ...
>   Call Trace:
>    netlbl_cipsov4_add_std+0xf4/0x8c0
>    netlbl_cipsov4_add+0x13f/0x1b0
>    genl_family_rcv_msg_doit.isra.15+0x132/0x170
>    genl_rcv_msg+0x125/0x240
> 
> [...]

Here is the summary with links:
  - [v2] net: fix NULL pointer reference in cipso_v4_doi_free
    https://git.kernel.org/netdev/net-next/c/e842cb60e8ac

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-30 10:28 ` [PATCH v2] " 王贇
  2021-08-30 11:30   ` patchwork-bot+netdevbpf
@ 2021-08-30 14:17   ` Paul Moore
  2021-08-30 16:45     ` Jakub Kicinski
  2021-09-01  2:18   ` [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free" 王贇
  2 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-08-30 14:17 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote:
>
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
>   BUG: kernel NULL pointer dereference, address:
>   ...
>   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
>   ...
>   Call Trace:
>    netlbl_cipsov4_add_std+0xf4/0x8c0
>    netlbl_cipsov4_add+0x13f/0x1b0
>    genl_family_rcv_msg_doit.isra.15+0x132/0x170
>    genl_rcv_msg+0x125/0x240
>
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when doi_def->type got value 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
>
> This patch just add the check to prevent panic happen in similar
> cases.
>
> Reported-by: Abaci <abaci@linux.alibaba.com>
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> ---
>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I see this was already merged, but it looks good to me, thanks for
making those changes.

> diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
> index baf2357..344c228 100644
> --- a/net/netlabel/netlabel_cipso_v4.c
> +++ b/net/netlabel/netlabel_cipso_v4.c
> @@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
>                 return -ENOMEM;
>         doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
>         if (doi_def->map.std == NULL) {
> -               ret_val = -ENOMEM;
> -               goto add_std_failure;
> +               kfree(doi_def);
> +               return -ENOMEM;
>         }
>         doi_def->type = CIPSO_V4_MAP_TRANS;
>
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-30 14:17   ` Paul Moore
@ 2021-08-30 16:45     ` Jakub Kicinski
  2021-08-30 16:50       ` Paul Moore
  0 siblings, 1 reply; 25+ messages in thread
From: Jakub Kicinski @ 2021-08-30 16:45 UTC (permalink / raw)
  To: Paul Moore
  Cc: 王贇,
	David S. Miller, Hideaki YOSHIFUJI, David Ahern, netdev,
	linux-security-module, linux-kernel

On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
> On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote:
> >
> > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> > failed, we sometime observe panic:
> >
> >   BUG: kernel NULL pointer dereference, address:
> >   ...
> >   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> >   ...
> >   Call Trace:
> >    netlbl_cipsov4_add_std+0xf4/0x8c0
> >    netlbl_cipsov4_add+0x13f/0x1b0
> >    genl_family_rcv_msg_doit.isra.15+0x132/0x170
> >    genl_rcv_msg+0x125/0x240
> >
> > This is because in cipso_v4_doi_free() there is no check
> > on 'doi_def->map.std' when doi_def->type got value 1, which
> > is possibe, since netlbl_cipsov4_add_std() haven't initialize
> > it before alloc 'doi_def->map.std'.
> >
> > This patch just add the check to prevent panic happen in similar
> > cases.
> >
> > Reported-by: Abaci <abaci@linux.alibaba.com>
> > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > ---
> >  net/netlabel/netlabel_cipso_v4.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)  
> 
> I see this was already merged, but it looks good to me, thanks for
> making those changes.

FWIW it looks like v1 was also merged:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-30 16:45     ` Jakub Kicinski
@ 2021-08-30 16:50       ` Paul Moore
  2021-08-31  2:41         ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-08-30 16:50 UTC (permalink / raw)
  To: Jakub Kicinski, 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, netdev,
	linux-security-module, linux-kernel

On Mon, Aug 30, 2021 at 12:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
> > On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote:
> > >
> > > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> > > failed, we sometime observe panic:
> > >
> > >   BUG: kernel NULL pointer dereference, address:
> > >   ...
> > >   RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> > >   ...
> > >   Call Trace:
> > >    netlbl_cipsov4_add_std+0xf4/0x8c0
> > >    netlbl_cipsov4_add+0x13f/0x1b0
> > >    genl_family_rcv_msg_doit.isra.15+0x132/0x170
> > >    genl_rcv_msg+0x125/0x240
> > >
> > > This is because in cipso_v4_doi_free() there is no check
> > > on 'doi_def->map.std' when doi_def->type got value 1, which
> > > is possibe, since netlbl_cipsov4_add_std() haven't initialize
> > > it before alloc 'doi_def->map.std'.
> > >
> > > This patch just add the check to prevent panic happen in similar
> > > cases.
> > >
> > > Reported-by: Abaci <abaci@linux.alibaba.com>
> > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> > > ---
> > >  net/netlabel/netlabel_cipso_v4.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > I see this was already merged, but it looks good to me, thanks for
> > making those changes.
>
> FWIW it looks like v1 was also merged:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b

Yeah, that is unfortunate, there was a brief discussion about that
over on one of the -stable patches for the v1 patch (odd that I never
saw a patchbot post for the v1 patch?).  Having both merged should be
harmless, but we want to revert the v1 patch as soon as we can.
Michael, can you take care of this?

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-30 16:50       ` Paul Moore
@ 2021-08-31  2:41         ` 王贇
  2021-08-31 13:48           ` Paul Moore
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-08-31  2:41 UTC (permalink / raw)
  To: Paul Moore, Jakub Kicinski
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, netdev,
	linux-security-module, linux-kernel



On 2021/8/31 上午12:50, Paul Moore wrote:
[SNIP]
>>>> Reported-by: Abaci <abaci@linux.alibaba.com>
>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>> ---
>>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> I see this was already merged, but it looks good to me, thanks for
>>> making those changes.
>>
>> FWIW it looks like v1 was also merged:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
> 
> Yeah, that is unfortunate, there was a brief discussion about that
> over on one of the -stable patches for the v1 patch (odd that I never
> saw a patchbot post for the v1 patch?).  Having both merged should be
> harmless, but we want to revert the v1 patch as soon as we can.
> Michael, can you take care of this?

As v1 already merged, may be we could just goon with it?

Actually both working to fix the problem, v1 will cover all the
cases, v2 take care one case since that's currently the only one,
but maybe there will be more in future.

Regards,
Michael Wang

> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-31  2:41         ` 王贇
@ 2021-08-31 13:48           ` Paul Moore
  2021-09-01  1:51             ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-08-31 13:48 UTC (permalink / raw)
  To: 王贇
  Cc: Jakub Kicinski, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	netdev, linux-security-module, linux-kernel

On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
> On 2021/8/31 上午12:50, Paul Moore wrote:
> [SNIP]
> >>>> Reported-by: Abaci <abaci@linux.alibaba.com>
> >>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> >>>> ---
> >>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> I see this was already merged, but it looks good to me, thanks for
> >>> making those changes.
> >>
> >> FWIW it looks like v1 was also merged:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
> >
> > Yeah, that is unfortunate, there was a brief discussion about that
> > over on one of the -stable patches for the v1 patch (odd that I never
> > saw a patchbot post for the v1 patch?).  Having both merged should be
> > harmless, but we want to revert the v1 patch as soon as we can.
> > Michael, can you take care of this?
>
> As v1 already merged, may be we could just goon with it?
>
> Actually both working to fix the problem, v1 will cover all the
> cases, v2 take care one case since that's currently the only one,
> but maybe there will be more in future.

No.  Please revert v1 and stick with the v2 patch.  The v1 patch is in
my opinion a rather ugly hack that addresses the symptom of the
problem and not the root cause.

It isn't your fault that both v1 and v2 were merged, but I'm asking
you to help cleanup the mess.  If you aren't able to do that please
let us know so that others can fix this properly.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-08-31 13:48           ` Paul Moore
@ 2021-09-01  1:51             ` 王贇
  2021-09-01  9:30               ` David Miller
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-01  1:51 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jakub Kicinski, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	netdev, linux-security-module, linux-kernel



On 2021/8/31 下午9:48, Paul Moore wrote:
> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>> On 2021/8/31 上午12:50, Paul Moore wrote:
>> [SNIP]
>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com>
>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>>>> ---
>>>>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>> making those changes.
>>>>
>>>> FWIW it looks like v1 was also merged:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>
>>> Yeah, that is unfortunate, there was a brief discussion about that
>>> over on one of the -stable patches for the v1 patch (odd that I never
>>> saw a patchbot post for the v1 patch?).  Having both merged should be
>>> harmless, but we want to revert the v1 patch as soon as we can.
>>> Michael, can you take care of this?
>>
>> As v1 already merged, may be we could just goon with it?
>>
>> Actually both working to fix the problem, v1 will cover all the
>> cases, v2 take care one case since that's currently the only one,
>> but maybe there will be more in future.
> 
> No.  Please revert v1 and stick with the v2 patch.  The v1 patch is in
> my opinion a rather ugly hack that addresses the symptom of the
> problem and not the root cause.
> 
> It isn't your fault that both v1 and v2 were merged, but I'm asking
> you to help cleanup the mess.  If you aren't able to do that please
> let us know so that others can fix this properly.

No problem I can help on that, just try to make sure it's not a
meaningless work.

So would it be fine to send out a v3 which revert v1 and apply v2?

Regards,
Michael Wang

> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-08-30 10:28 ` [PATCH v2] " 王贇
  2021-08-30 11:30   ` patchwork-bot+netdevbpf
  2021-08-30 14:17   ` Paul Moore
@ 2021-09-01  2:18   ` 王贇
  2021-09-01  2:21     ` 王贇
  2 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-01  2:18 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.

Since commit e842cb60e8ac ("net: fix NULL pointer reference in
cipso_v4_doi_free") also applied to fix the root cause, we can
just revert the old version now.

Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 net/ipv4/cipso_ipv4.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 7fbd0b5..099259f 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
 	if (!doi_def)
 		return;

-	if (doi_def->map.std) {
-		switch (doi_def->type) {
-		case CIPSO_V4_MAP_TRANS:
-			kfree(doi_def->map.std->lvl.cipso);
-			kfree(doi_def->map.std->lvl.local);
-			kfree(doi_def->map.std->cat.cipso);
-			kfree(doi_def->map.std->cat.local);
-			kfree(doi_def->map.std);
-			break;
-		}
+	switch (doi_def->type) {
+	case CIPSO_V4_MAP_TRANS:
+		kfree(doi_def->map.std->lvl.cipso);
+		kfree(doi_def->map.std->lvl.local);
+		kfree(doi_def->map.std->cat.cipso);
+		kfree(doi_def->map.std->cat.local);
+		kfree(doi_def->map.std);
+		break;
 	}
 	kfree(doi_def);
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-01  2:18   ` [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free" 王贇
@ 2021-09-01  2:21     ` 王贇
  2021-09-01 21:05       ` Paul Moore
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-01  2:21 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?

Regards,
Michael Wang

On 2021/9/1 上午10:18, 王贇 wrote:
> This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.
> 
> Since commit e842cb60e8ac ("net: fix NULL pointer reference in
> cipso_v4_doi_free") also applied to fix the root cause, we can
> just revert the old version now.
> 
> Suggested-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> ---
>  net/ipv4/cipso_ipv4.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 7fbd0b5..099259f 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>  	if (!doi_def)
>  		return;
> 
> -	if (doi_def->map.std) {
> -		switch (doi_def->type) {
> -		case CIPSO_V4_MAP_TRANS:
> -			kfree(doi_def->map.std->lvl.cipso);
> -			kfree(doi_def->map.std->lvl.local);
> -			kfree(doi_def->map.std->cat.cipso);
> -			kfree(doi_def->map.std->cat.local);
> -			kfree(doi_def->map.std);
> -			break;
> -		}
> +	switch (doi_def->type) {
> +	case CIPSO_V4_MAP_TRANS:
> +		kfree(doi_def->map.std->lvl.cipso);
> +		kfree(doi_def->map.std->lvl.local);
> +		kfree(doi_def->map.std->cat.cipso);
> +		kfree(doi_def->map.std->cat.local);
> +		kfree(doi_def->map.std);
> +		break;
>  	}
>  	kfree(doi_def);
>  }
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-09-01  1:51             ` 王贇
@ 2021-09-01  9:30               ` David Miller
  2021-09-01  9:41                 ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: David Miller @ 2021-09-01  9:30 UTC (permalink / raw)
  To: yun.wang
  Cc: paul, kuba, yoshfuji, dsahern, netdev, linux-security-module,
	linux-kernel

From: 王贇 <yun.wang@linux.alibaba.com>
Date: Wed, 1 Sep 2021 09:51:28 +0800

> 
> 
> On 2021/8/31 下午9:48, Paul Moore wrote:
>> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>>> On 2021/8/31 上午12:50, Paul Moore wrote:
>>> [SNIP]
>>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com>
>>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>>>>> ---
>>>>>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>> making those changes.
>>>>>
>>>>> FWIW it looks like v1 was also merged:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>
>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>> saw a patchbot post for the v1 patch?).  Having both merged should be
>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>> Michael, can you take care of this?
>>>
>>> As v1 already merged, may be we could just goon with it?
>>>
>>> Actually both working to fix the problem, v1 will cover all the
>>> cases, v2 take care one case since that's currently the only one,
>>> but maybe there will be more in future.
>> 
>> No.  Please revert v1 and stick with the v2 patch.  The v1 patch is in
>> my opinion a rather ugly hack that addresses the symptom of the
>> problem and not the root cause.
>> 
>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>> you to help cleanup the mess.  If you aren't able to do that please
>> let us know so that others can fix this properly.
> 
> No problem I can help on that, just try to make sure it's not a
> meaningless work.
> 
> So would it be fine to send out a v3 which revert v1 and apply v2?

Please don't do things this way just send the relative change.

Thanks.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-09-01  9:30               ` David Miller
@ 2021-09-01  9:41                 ` 王贇
  2021-09-01 10:45                   ` David Miller
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-01  9:41 UTC (permalink / raw)
  To: David Miller
  Cc: paul, kuba, yoshfuji, dsahern, netdev, linux-security-module,
	linux-kernel



On 2021/9/1 下午5:30, David Miller wrote:
> From: 王贇 <yun.wang@linux.alibaba.com>
> Date: Wed, 1 Sep 2021 09:51:28 +0800
> 
>>
>>
>> On 2021/8/31 下午9:48, Paul Moore wrote:
>>> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>>>> On 2021/8/31 上午12:50, Paul Moore wrote:
>>>> [SNIP]
>>>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com>
>>>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>>>>>> ---
>>>>>>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>>> making those changes.
>>>>>>
>>>>>> FWIW it looks like v1 was also merged:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>>
>>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>>> saw a patchbot post for the v1 patch?).  Having both merged should be
>>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>>> Michael, can you take care of this?
>>>>
>>>> As v1 already merged, may be we could just goon with it?
>>>>
>>>> Actually both working to fix the problem, v1 will cover all the
>>>> cases, v2 take care one case since that's currently the only one,
>>>> but maybe there will be more in future.
>>>
>>> No.  Please revert v1 and stick with the v2 patch.  The v1 patch is in
>>> my opinion a rather ugly hack that addresses the symptom of the
>>> problem and not the root cause.
>>>
>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>> you to help cleanup the mess.  If you aren't able to do that please
>>> let us know so that others can fix this properly.
>>
>> No problem I can help on that, just try to make sure it's not a
>> meaningless work.
>>
>> So would it be fine to send out a v3 which revert v1 and apply v2?
> 
> Please don't do things this way just send the relative change.

Could you please check the patch:

Revert "net: fix NULL pointer reference in cipso_v4_doi_free"

see if that's the right way?

Regards,
Michael Wang

> 
> Thanks.
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-09-01  9:41                 ` 王贇
@ 2021-09-01 10:45                   ` David Miller
  2021-09-02  3:04                     ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: David Miller @ 2021-09-01 10:45 UTC (permalink / raw)
  To: yun.wang
  Cc: paul, kuba, yoshfuji, dsahern, netdev, linux-security-module,
	linux-kernel

From: 王贇 <yun.wang@linux.alibaba.com>
Date: Wed, 1 Sep 2021 17:41:00 +0800

> 
> 
> On 2021/9/1 下午5:30, David Miller wrote:
>> From: 王贇 <yun.wang@linux.alibaba.com>
>> Date: Wed, 1 Sep 2021 09:51:28 +0800
>> 
>>>
>>>
>>> On 2021/8/31 下午9:48, Paul Moore wrote:
>>>> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>>>>> On 2021/8/31 上午12:50, Paul Moore wrote:
>>>>> [SNIP]
>>>>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com>
>>>>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>>>>>>>>> ---
>>>>>>>>>  net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>>>> making those changes.
>>>>>>>
>>>>>>> FWIW it looks like v1 was also merged:
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>>>
>>>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>>>> saw a patchbot post for the v1 patch?).  Having both merged should be
>>>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>>>> Michael, can you take care of this?
>>>>>
>>>>> As v1 already merged, may be we could just goon with it?
>>>>>
>>>>> Actually both working to fix the problem, v1 will cover all the
>>>>> cases, v2 take care one case since that's currently the only one,
>>>>> but maybe there will be more in future.
>>>>
>>>> No.  Please revert v1 and stick with the v2 patch.  The v1 patch is in
>>>> my opinion a rather ugly hack that addresses the symptom of the
>>>> problem and not the root cause.
>>>>
>>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>>> you to help cleanup the mess.  If you aren't able to do that please
>>>> let us know so that others can fix this properly.
>>>
>>> No problem I can help on that, just try to make sure it's not a
>>> meaningless work.
>>>
>>> So would it be fine to send out a v3 which revert v1 and apply v2?
>> 
>> Please don't do things this way just send the relative change.
> 
> Could you please check the patch:
> 
> Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
> 
> see if that's the right way?

It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change.

Thank you.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-01  2:21     ` 王贇
@ 2021-09-01 21:05       ` Paul Moore
  2021-09-02  2:37         ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-09-01 21:05 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Tue, Aug 31, 2021 at 10:21 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>
> Hi Paul, it confused me since it's the first time I face
> such situation, but I just realized what you're asking is
> actually this revert, correct?

I believe DaveM already answered your question in the other thread,
but if you are still unsure about the patch let me know.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-01 21:05       ` Paul Moore
@ 2021-09-02  2:37         ` 王贇
  2021-09-03  2:15           ` Paul Moore
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-02  2:37 UTC (permalink / raw)
  To: Paul Moore
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel



On 2021/9/2 上午5:05, Paul Moore wrote:
> On Tue, Aug 31, 2021 at 10:21 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>>
>> Hi Paul, it confused me since it's the first time I face
>> such situation, but I just realized what you're asking is
>> actually this revert, correct?
> 
> I believe DaveM already answered your question in the other thread,
> but if you are still unsure about the patch let me know.

I do failed to get the point :-(

As I checked the:
  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

both v1 and v2 are there with the same description and both code modification
are applied.

We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?

Regards,
Michael Wang

> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] net: fix NULL pointer reference in cipso_v4_doi_free
  2021-09-01 10:45                   ` David Miller
@ 2021-09-02  3:04                     ` 王贇
  0 siblings, 0 replies; 25+ messages in thread
From: 王贇 @ 2021-09-02  3:04 UTC (permalink / raw)
  To: David Miller
  Cc: paul, kuba, yoshfuji, dsahern, netdev, linux-security-module,
	linux-kernel



On 2021/9/1 下午6:45, David Miller wrote:
[snip]
>>>>>
>>>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>>>> you to help cleanup the mess.  If you aren't able to do that please
>>>>> let us know so that others can fix this properly.
>>>>
>>>> No problem I can help on that, just try to make sure it's not a
>>>> meaningless work.
>>>>
>>>> So would it be fine to send out a v3 which revert v1 and apply v2?
>>>
>>> Please don't do things this way just send the relative change.
>>
>> Could you please check the patch:
>>
>> Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
>>
>> see if that's the right way?
> 
> It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change.

Sorry for my horrible reading comprehension... I checked netdev/net.git master branch
and saw v2's change already applied, thus I've no idea how to change it again but pretty
sure I still misunderstanding the suggestion, could please kindly provide more details?

Regards,
Michael Wang

> 
> Thank you.
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-02  2:37         ` 王贇
@ 2021-09-03  2:15           ` Paul Moore
  2021-09-03  2:31             ` 王贇
  0 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2021-09-03  2:15 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Wed, Sep 1, 2021 at 10:37 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
> On 2021/9/2 上午5:05, Paul Moore wrote:
> > On Tue, Aug 31, 2021 at 10:21 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
> >>
> >> Hi Paul, it confused me since it's the first time I face
> >> such situation, but I just realized what you're asking is
> >> actually this revert, correct?
> >
> > I believe DaveM already answered your question in the other thread,
> > but if you are still unsure about the patch let me know.
>
> I do failed to get the point :-(
>
> As I checked the:
>   https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
>
> both v1 and v2 are there with the same description and both code modification
> are applied.
>
> We want revert v1 but not in a revert patch style, then do you suggest
> send a normal patch to do the code revert?

It sounds like DaveM wants you to create a normal (not a revert) patch
that removes the v1 changes while leaving the v2 changes intact.  In
the patch description you can mention that v1 was merged as a mistake
and that v2 is the correct fix (provide commit IDs for each in your
commit description using the usual 12-char hash snippet followed by
the subject in parens-and-quotes).

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] net: remove the unnecessary check in cipso_v4_doi_free
  2021-08-26  3:42 [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free 王贇
                   ` (2 preceding siblings ...)
  2021-08-30 10:28 ` [PATCH v2] " 王贇
@ 2021-09-03  2:27 ` 王贇
  2021-09-03 14:08   ` Paul Moore
  3 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-03  2:27 UTC (permalink / raw)
  To: Paul Moore, David S. Miller, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, netdev, linux-security-module, linux-kernel

The commit 733c99ee8be9 ("net: fix NULL pointer reference in
cipso_v4_doi_free") was merged by a mistake, this patch try
to cleanup the mess.

And we already have the commit e842cb60e8ac ("net: fix NULL
pointer reference in cipso_v4_doi_free") which fixed the root
cause of the issue mentioned in it's description.

Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
---
 net/ipv4/cipso_ipv4.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 7fbd0b5..099259f 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
 	if (!doi_def)
 		return;

-	if (doi_def->map.std) {
-		switch (doi_def->type) {
-		case CIPSO_V4_MAP_TRANS:
-			kfree(doi_def->map.std->lvl.cipso);
-			kfree(doi_def->map.std->lvl.local);
-			kfree(doi_def->map.std->cat.cipso);
-			kfree(doi_def->map.std->cat.local);
-			kfree(doi_def->map.std);
-			break;
-		}
+	switch (doi_def->type) {
+	case CIPSO_V4_MAP_TRANS:
+		kfree(doi_def->map.std->lvl.cipso);
+		kfree(doi_def->map.std->lvl.local);
+		kfree(doi_def->map.std->cat.cipso);
+		kfree(doi_def->map.std->cat.local);
+		kfree(doi_def->map.std);
+		break;
 	}
 	kfree(doi_def);
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-03  2:15           ` Paul Moore
@ 2021-09-03  2:31             ` 王贇
  2021-09-03 14:08               ` Paul Moore
  0 siblings, 1 reply; 25+ messages in thread
From: 王贇 @ 2021-09-03  2:31 UTC (permalink / raw)
  To: Paul Moore
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel



On 2021/9/3 上午10:15, Paul Moore wrote:
[snip]
>> both v1 and v2 are there with the same description and both code modification
>> are applied.
>>
>> We want revert v1 but not in a revert patch style, then do you suggest
>> send a normal patch to do the code revert?
> 
> It sounds like DaveM wants you to create a normal (not a revert) patch
> that removes the v1 changes while leaving the v2 changes intact.  In
> the patch description you can mention that v1 was merged as a mistake
> and that v2 is the correct fix (provide commit IDs for each in your
> commit description using the usual 12-char hash snippet followed by
> the subject in parens-and-quotes).

Thanks for the kindly explain, I've sent:
  [PATCH] net: remove the unnecessary check in cipso_v4_doi_free

Which actually revert the v1 and mentioned v2 fixed the root casue,
Would you please take a look see if that is helpful?

Regards,
Michael Wang

> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] net: remove the unnecessary check in cipso_v4_doi_free
  2021-09-03  2:27 ` [PATCH] net: remove the unnecessary check in cipso_v4_doi_free 王贇
@ 2021-09-03 14:08   ` Paul Moore
  0 siblings, 0 replies; 25+ messages in thread
From: Paul Moore @ 2021-09-03 14:08 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Thu, Sep 2, 2021 at 10:27 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
>
> The commit 733c99ee8be9 ("net: fix NULL pointer reference in
> cipso_v4_doi_free") was merged by a mistake, this patch try
> to cleanup the mess.
>
> And we already have the commit e842cb60e8ac ("net: fix NULL
> pointer reference in cipso_v4_doi_free") which fixed the root
> cause of the issue mentioned in it's description.
>
> Suggested-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
> ---
>  net/ipv4/cipso_ipv4.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

Verified that the v2 patch is in net/master so removing the v1 patch
as this does is the right thing to do.  Thanks for sending this out.

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 7fbd0b5..099259f 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>         if (!doi_def)
>                 return;
>
> -       if (doi_def->map.std) {
> -               switch (doi_def->type) {
> -               case CIPSO_V4_MAP_TRANS:
> -                       kfree(doi_def->map.std->lvl.cipso);
> -                       kfree(doi_def->map.std->lvl.local);
> -                       kfree(doi_def->map.std->cat.cipso);
> -                       kfree(doi_def->map.std->cat.local);
> -                       kfree(doi_def->map.std);
> -                       break;
> -               }
> +       switch (doi_def->type) {
> +       case CIPSO_V4_MAP_TRANS:
> +               kfree(doi_def->map.std->lvl.cipso);
> +               kfree(doi_def->map.std->lvl.local);
> +               kfree(doi_def->map.std->cat.cipso);
> +               kfree(doi_def->map.std->cat.local);
> +               kfree(doi_def->map.std);
> +               break;
>         }
>         kfree(doi_def);
>  }
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
  2021-09-03  2:31             ` 王贇
@ 2021-09-03 14:08               ` Paul Moore
  0 siblings, 0 replies; 25+ messages in thread
From: Paul Moore @ 2021-09-03 14:08 UTC (permalink / raw)
  To: 王贇
  Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	netdev, linux-security-module, linux-kernel

On Thu, Sep 2, 2021 at 10:31 PM 王贇 <yun.wang@linux.alibaba.com> wrote:
> On 2021/9/3 上午10:15, Paul Moore wrote:
> [snip]
> >> both v1 and v2 are there with the same description and both code modification
> >> are applied.
> >>
> >> We want revert v1 but not in a revert patch style, then do you suggest
> >> send a normal patch to do the code revert?
> >
> > It sounds like DaveM wants you to create a normal (not a revert) patch
> > that removes the v1 changes while leaving the v2 changes intact.  In
> > the patch description you can mention that v1 was merged as a mistake
> > and that v2 is the correct fix (provide commit IDs for each in your
> > commit description using the usual 12-char hash snippet followed by
> > the subject in parens-and-quotes).
>
> Thanks for the kindly explain, I've sent:
>   [PATCH] net: remove the unnecessary check in cipso_v4_doi_free
>
> Which actually revert the v1 and mentioned v2 fixed the root casue,
> Would you please take a look see if that is helpful?

That looks correct to me, acked.  Thanks.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-09-03 14:09 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26  3:42 [PATCH] net: fix NULL pointer reference in cipso_v4_doi_free 王贇
2021-08-27  0:09 ` Paul Moore
2021-08-30 10:20   ` 王贇
2021-08-30 10:14 ` 王贇
2021-08-30 10:28 ` [PATCH v2] " 王贇
2021-08-30 11:30   ` patchwork-bot+netdevbpf
2021-08-30 14:17   ` Paul Moore
2021-08-30 16:45     ` Jakub Kicinski
2021-08-30 16:50       ` Paul Moore
2021-08-31  2:41         ` 王贇
2021-08-31 13:48           ` Paul Moore
2021-09-01  1:51             ` 王贇
2021-09-01  9:30               ` David Miller
2021-09-01  9:41                 ` 王贇
2021-09-01 10:45                   ` David Miller
2021-09-02  3:04                     ` 王贇
2021-09-01  2:18   ` [PATCH] Revert "net: fix NULL pointer reference in cipso_v4_doi_free" 王贇
2021-09-01  2:21     ` 王贇
2021-09-01 21:05       ` Paul Moore
2021-09-02  2:37         ` 王贇
2021-09-03  2:15           ` Paul Moore
2021-09-03  2:31             ` 王贇
2021-09-03 14:08               ` Paul Moore
2021-09-03  2:27 ` [PATCH] net: remove the unnecessary check in cipso_v4_doi_free 王贇
2021-09-03 14:08   ` Paul Moore

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).