net: cipso: bug fixes

PATCH Superseded Pavel Skripkin 3 participants 9 messages Jul 10, 2021 – Jul 27, 2021

Subsystems (4)

Networking Network drivers Security (LSM) Labeled networking (NetLabel)

lore

Share & tools

lore

Pavel Skripkin

author

2021-07-10 07:03 UTC #0 lore raw

cover net: cipso: bug fixes

This small patch series fixes 2 bugs in cipso code. The first one was found by syzbot and the second was found while testing fix for the fisrt one. All 2 bugs can be triggered by reproducer provided by syzbot:

https://syzkaller.appspot.com/bug?id=d4bc7d67efe79c6ead3cb6bd94b84dbd287f1069

Pavel Skripkin (2):
net: cipso: fix warnings in netlbl_cipsov4_add_std
net: cipso: fix memory leak in cipso_v4_doi_free

 net/ipv4/cipso_ipv4.c            | 1 +
 net/netlabel/netlabel_cipso_v4.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

Signature · 1 line

2.32.0

Pavel Skripkin

author

2021-07-10 07:03 UTC #1 lore raw

PATCH 1/2 net: cipso: fix warnings in netlbl_cipsov4_add_std

Syzbot reported warning in netlbl_cipsov4_add(). The problem was in too big doi_def->map.std->lvl.local_size passed to kcalloc(). Since this value comes from userpace there is no need to warn if value is not correct.

The same problem may occur with other kcalloc() calls in this function, so, I've added __GFP_NOWARN flag to all kcalloc() calls there.

Reported-and-tested-by [email protected]

Fixes 96cb8e3313c7 ("[NetLabel]: CIPSOv4 and Unlabeled packet integration")

Signed-off-by Pavel Skripkin <[email protected]>

---
 net/netlabel/netlabel_cipso_v4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

net/netlabel/netlabel_cipso_v4.c +4 -4

index 4f50a64315cf..50f40943c815 100644
@@ -187,14 +187,14 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
 		}
 	doi_def->map.std->lvl.local = kcalloc(doi_def->map.std->lvl.local_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL | __GFP_NOWARN);
 	if (doi_def->map.std->lvl.local == NULL) {
 		ret_val = -ENOMEM;
 		goto add_std_failure;
 	}
 	doi_def->map.std->lvl.cipso = kcalloc(doi_def->map.std->lvl.cipso_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL | __GFP_NOWARN);
 	if (doi_def->map.std->lvl.cipso == NULL) {
 		ret_val = -ENOMEM;
 		goto add_std_failure;

@@ -263,7 +263,7 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
 		doi_def->map.std->cat.local = kcalloc(
 					      doi_def->map.std->cat.local_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL | __GFP_NOWARN);
 		if (doi_def->map.std->cat.local == NULL) {
 			ret_val = -ENOMEM;
 			goto add_std_failure;

@@ -271,7 +271,7 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
 		doi_def->map.std->cat.cipso = kcalloc(
 					      doi_def->map.std->cat.cipso_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL | __GFP_NOWARN);
 		if (doi_def->map.std->cat.cipso == NULL) {
 			ret_val = -ENOMEM;
 			goto add_std_failure;

Signature · 1 line

2.32.0

Paul Moore

maintainer Acked-by on v1 1/2

2021-07-12 15:03 UTC #2 lore raw

Re: [PATCH 1/2] net: cipso: fix warnings in netlbl_cipsov4_add_std

Quoted 10 lines · Pavel Skripkin

Syzbot reported warning in netlbl_cipsov4_add(). The problem was in too big doi_def->map.std->lvl.local_size passed to kcalloc(). Since this value comes from userpace there is no need to warn if value is not correct.
The same problem may occur with other kcalloc() calls in this function, so, I've added __GFP_NOWARN flag to all kcalloc() calls there.

Quoted 6 lines

Reported-and-tested-by [email protected]
Fixes 96cb8e3313c7 ("[NetLabel]: CIPSOv4 and Unlabeled packet integration")
Signed-off-by Pavel Skripkin <[email protected]>
---
 net/netlabel/netlabel_cipso_v4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

This seems fine to me, callers will get a ENOMEM error code too so it isn't like the failure is going to be a mystery, especially in the case where an obscenely large translation mapping is being attempted.

Acked-by Paul Moore <[email protected]>

As an aside, I see no reason why this patch can't be merged and 2/2 simply dropped as already in-tree. As has already been pointed out, patch 2/2 is a duplicate; the in-tree commit is d612c3f3fae2 ("net:
ipv4: fix memory leak in netlbl_cipsov4_add_std").

Quoted 41 lines

net/netlabel/netlabel_cipso_v4.c +4 -5

index 4f50a64315cf..50f40943c815 100644
@@ -187,14 +187,14 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
                }
        doi_def->map.std->lvl.local = kcalloc(doi_def->map.std->lvl.local_size,
                                              sizeof(u32),
-                                             GFP_KERNEL);
+                                             GFP_KERNEL | __GFP_NOWARN);
        if (doi_def->map.std->lvl.local == NULL) {
                ret_val = -ENOMEM;
                goto add_std_failure;
        }
        doi_def->map.std->lvl.cipso = kcalloc(doi_def->map.std->lvl.cipso_size,
                                              sizeof(u32),
-                                             GFP_KERNEL);
+                                             GFP_KERNEL | __GFP_NOWARN);
        if (doi_def->map.std->lvl.cipso == NULL) {
                ret_val = -ENOMEM;
                goto add_std_failure;

@@ -263,7 +263,7 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
                doi_def->map.std->cat.local = kcalloc(
                                              doi_def->map.std->cat.local_size,
                                              sizeof(u32),
-                                             GFP_KERNEL);
+                                             GFP_KERNEL | __GFP_NOWARN);
                if (doi_def->map.std->cat.local == NULL) {
                        ret_val = -ENOMEM;
                        goto add_std_failure;

@@ -271,7 +271,7 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
                doi_def->map.std->cat.cipso = kcalloc(
                                              doi_def->map.std->cat.cipso_size,
                                              sizeof(u32),
-                                             GFP_KERNEL);
+                                             GFP_KERNEL | __GFP_NOWARN);
                if (doi_def->map.std->cat.cipso == NULL) {
                        ret_val = -ENOMEM;
                        goto add_std_failure;
--

2.32.0

Signature · 2 lines

paul moore
www.paul-moore.com

Pavel Skripkin

author

2021-07-26 11:11 UTC #3 lore raw

Re: [PATCH 1/2] net: cipso: fix warnings in netlbl_cipsov4_add_std

In reply to Pavel Skripkin

Quoted 53 lines · On Sat, 10 Jul 2021 10:03:13 +0300 Pavel Skripkin <[email protected]> wrote:

Syzbot reported warning in netlbl_cipsov4_add(). The problem was in too big doi_def->map.std->lvl.local_size passed to kcalloc(). Since this value comes from userpace there is no need to warn if value is not correct.
The same problem may occur with other kcalloc() calls in this function, so, I've added __GFP_NOWARN flag to all kcalloc() calls there.
Reported-and-tested-by:
[email protected] Fixes:
96cb8e3313c7 ("[NetLabel]: CIPSOv4 and Unlabeled packet integration")
Signed-off-by Pavel Skripkin <[email protected]> ---
 net/netlabel/netlabel_cipso_v4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/net/netlabel/netlabel_cipso_v4.c index 4f50a64315cf..50f40943c815 100644 --- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
(quoted diff) +2 -2
@@ -187,14 +187,14 @@ static int netlbl_cipsov4_add_std(struct
 genl_info *info, }
 	doi_def->map.std->lvl.local =
 kcalloc(doi_def->map.std->lvl.local_size, sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL |
 __GFP_NOWARN); if (doi_def->map.std->lvl.local == NULL) {
 		ret_val = -ENOMEM;
 		goto add_std_failure;
 	}
 	doi_def->map.std->lvl.cipso =
 kcalloc(doi_def->map.std->lvl.cipso_size, sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL |
 __GFP_NOWARN); if (doi_def->map.std->lvl.cipso == NULL) {
 		ret_val = -ENOMEM;
 		goto add_std_failure;
@@ -263,7 +263,7 @@ static int netlbl_cipsov4_add_std(struct
genl_info *info, doi_def->map.std->cat.local = kcalloc(
net/netlabel/netlabel_cipso_v4.c +1 -1
 					      doi_def->map.std->cat.local_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL |
__GFP_NOWARN); if (doi_def->map.std->cat.local == NULL) {
(quoted diff) +0 -0
 			ret_val = -ENOMEM;
 			goto add_std_failure;
@@ -271,7 +271,7 @@ static int netlbl_cipsov4_add_std(struct
genl_info *info, doi_def->map.std->cat.cipso = kcalloc(
net/netlabel/netlabel_cipso_v4.c +1 -1
 					      doi_def->map.std->cat.cipso_size,
 					      sizeof(u32),
-					      GFP_KERNEL);
+					      GFP_KERNEL |
__GFP_NOWARN); if (doi_def->map.std->cat.cipso == NULL) {

Review

Quoted 2 lines

(quoted diff) +0 -0

 			ret_val = -ENOMEM;
 			goto add_std_failure;

Hi, net developers!

Is this patch merged somewhere? I've checked net tree and Paul Moore tree on https://git.kernel.org/, but didn't find it. Did I miss it somewhere? If not, it's just a gentle ping :)

Btw: maybe I should send it as separete patch, since 2/2 in this series is invalid as already in-tree?

With regards,
Pavel Skripkin

Paul Moore

maintainer

2021-07-27 02:40 UTC #4 lore raw

Re: [PATCH 1/2] net: cipso: fix warnings in netlbl_cipsov4_add_std

Quoted 19 lines · Pavel Skripkin

Quoted 9 lines · On Sat, 10 Jul 2021 10:03:13 +0300 Pavel Skripkin <[email protected]> wrote:
Syzbot reported warning in netlbl_cipsov4_add(). The problem was in too big doi_def->map.std->lvl.local_size passed to kcalloc(). Since this value comes from userpace there is no need to warn if value is not correct.
The same problem may occur with other kcalloc() calls in this function, so, I've added __GFP_NOWARN flag to all kcalloc() calls there.
Quoted 6 lines
Reported-and-tested-by:
[email protected] Fixes:
96cb8e3313c7 ("[NetLabel]: CIPSOv4 and Unlabeled packet integration")
Signed-off-by Pavel Skripkin <[email protected]> ---
 net/netlabel/netlabel_cipso_v4.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 8 lines

Hi, net developers!
Is this patch merged somewhere? I've checked net tree and Paul Moore tree on https://git.kernel.org/, but didn't find it. Did I miss it somewhere? If not, it's just a gentle ping :)
Btw: maybe I should send it as separete patch, since 2/2 in this series is invalid as already in-tree?

I'm not sure why this hasn't been picked up yet, but I suppose resubmitting just this patch couldn't hurt. Don't forget to include my ACK if you do.

Signature · 2 lines

paul moore
www.paul-moore.com

Pavel Skripkin

author

2021-07-10 07:03 UTC #5 lore raw

PATCH 2/2 net: cipso: fix memory leak in cipso_v4_doi_free

In reply to Pavel Skripkin

When doi_def->type == CIPSO_V4_MAP_TRANS doi_def->map.std should be freed to avoid memory leak.

Fail log:

BUG: memory leak

unreferenced object 0xffff88801b936d00 (size 64):
comm "a.out", pid 8478, jiffies 4295042353 (age 15.260s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 15 b8 12 26 00 00 00 00 00 00 00 00 .......&........
backtrace:
netlbl_cipsov4_add (net/netlabel/netlabel_cipso_v4.c:145 net/netlabel/netlabel_cipso_v4.c:416) genl_family_rcv_msg_doit (net/netlink/genetlink.c:741)
genl_rcv_msg (net/netlink/genetlink.c:783 net/netlink/genetlink.c:800) netlink_rcv_skb (net/netlink/af_netlink.c:2505)
genl_rcv (net/netlink/genetlink.c:813)

Fixes b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with refrerence

counts")

Signed-off-by Pavel Skripkin <[email protected]>

---
 net/ipv4/cipso_ipv4.c | 1 +
 1 file changed, 1 insertion(+)

net/ipv4/cipso_ipv4.c +1 -0

index bfaf327e9d12..e0480c6cebaa 100644
@@ -472,6 +472,7 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
 		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);

Signature · 1 line

2.32.0

Dongliang Mu

2021-07-10 07:29 UTC #6 lore raw

Re: [PATCH 2/2] net: cipso: fix memory leak in cipso_v4_doi_free

Quoted 31 lines · Pavel Skripkin

When doi_def->type == CIPSO_V4_MAP_TRANS doi_def->map.std should be freed to avoid memory leak.
Fail log:
BUG: memory leak
unreferenced object 0xffff88801b936d00 (size 64):
comm "a.out", pid 8478, jiffies 4295042353 (age 15.260s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 15 b8 12 26 00 00 00 00 00 00 00 00 .......&........
backtrace:
netlbl_cipsov4_add (net/netlabel/netlabel_cipso_v4.c:145 net/netlabel/netlabel_cipso_v4.c:416) genl_family_rcv_msg_doit (net/netlink/genetlink.c:741)
genl_rcv_msg (net/netlink/genetlink.c:783 net/netlink/genetlink.c:800) netlink_rcv_skb (net/netlink/af_netlink.c:2505)
genl_rcv (net/netlink/genetlink.c:813)
Fixes b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with refrerence
counts")
Signed-off-by Pavel Skripkin <[email protected]>
---
 net/ipv4/cipso_ipv4.c | 1 +
 1 file changed, 1 insertion(+)
net/ipv4/cipso_ipv4.c +0 -0
index bfaf327e9d12..e0480c6cebaa 100644

net/ipv4/cipso_ipv4.c void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)

Quoted 8 lines

net/ipv4/cipso_ipv4.c +1 -1

@@ -472,6 +472,7 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
                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);
--

Hi Paval,

this patch is already merged by Paul. See [1] for more details.

[1] https://lore.kernel.org/netdev/CAHC9VhQZVOmy7n14nTSRGHzwN-y=E_JTUP+NpRCgD8rJN5sOGA@mail.gmail.com/T/

Quoted 2 lines

2.32.0

Pavel Skripkin

author

2021-07-10 07:43 UTC #7 lore raw

Re: [PATCH 2/2] net: cipso: fix memory leak in cipso_v4_doi_free

Quoted 45 lines · On Sat, 10 Jul 2021 15:29:19 +0800 Dongliang Mu <[email protected]>…

Quoted 32 lines · Pavel Skripkin
When doi_def->type == CIPSO_V4_MAP_TRANS doi_def->map.std should be freed to avoid memory leak.
Fail log:
BUG: memory leak
unreferenced object 0xffff88801b936d00 (size 64):
comm "a.out", pid 8478, jiffies 4295042353 (age 15.260s) hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 15 b8 12 26 00 00 00 00 00 00 00 00 .......&........ backtrace:
netlbl_cipsov4_add (net/netlabel/netlabel_cipso_v4.c:145 net/netlabel/netlabel_cipso_v4.c:416) genl_family_rcv_msg_doit (net/netlink/genetlink.c:741) genl_rcv_msg (net/netlink/genetlink.c:783 net/netlink/genetlink.c:800) netlink_rcv_skb (net/netlink/af_netlink.c:2505) genl_rcv (net/netlink/genetlink.c:813)
Fixes b1edeb102397 ("netlabel: Replace protocol/NetLabel linking
with refrerence counts")
Signed-off-by Pavel Skripkin <[email protected]>
---
 net/ipv4/cipso_ipv4.c | 1 +
 1 file changed, 1 insertion(+)
net/ipv4/cipso_ipv4.c +0 -0
index bfaf327e9d12..e0480c6cebaa 100644
net/ipv4/cipso_ipv4.c void cipso_v4_doi_free(struct cipso_v4_doi
Quoted 8 lines
net/ipv4/cipso_ipv4.c +1 -1
@@ -472,6 +472,7 @@ void cipso_v4_doi_free(struct cipso_v4_doi
 *doi_def) 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);
--
Hi Paval,

Quoted 5 lines

this patch is already merged by Paul. See [1] for more details.
[1]
https://lore.kernel.org/netdev/CAHC9VhQZVOmy7n14nTSRGHzwN-y=E_JTUP+NpRCgD8rJN5sOGA@mail.gmail.com/T/

Hi, Dongliang!

Thank you for the information. I'm wondering, can maintainer pick only 1 patch from series, or I should send v2?

With regards,
Pavel Skripkin

Dongliang Mu

2021-07-10 07:43 UTC #8 lore raw

Re: [PATCH 2/2] net: cipso: fix memory leak in cipso_v4_doi_free

Quoted 56 lines · Pavel Skripkin

Quoted 45 lines · On Sat, 10 Jul 2021 15:29:19 +0800 Dongliang Mu <[email protected]>…
Quoted 32 lines · Pavel Skripkin
When doi_def->type == CIPSO_V4_MAP_TRANS doi_def->map.std should be freed to avoid memory leak.
Fail log:
BUG: memory leak
unreferenced object 0xffff88801b936d00 (size 64):
comm "a.out", pid 8478, jiffies 4295042353 (age 15.260s) hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 15 b8 12 26 00 00 00 00 00 00 00 00 .......&........ backtrace:
netlbl_cipsov4_add (net/netlabel/netlabel_cipso_v4.c:145 net/netlabel/netlabel_cipso_v4.c:416) genl_family_rcv_msg_doit (net/netlink/genetlink.c:741) genl_rcv_msg (net/netlink/genetlink.c:783 net/netlink/genetlink.c:800) netlink_rcv_skb (net/netlink/af_netlink.c:2505) genl_rcv (net/netlink/genetlink.c:813)
Fixes b1edeb102397 ("netlabel: Replace protocol/NetLabel linking
with refrerence counts")
Signed-off-by Pavel Skripkin <[email protected]>
---
 net/ipv4/cipso_ipv4.c | 1 +
 1 file changed, 1 insertion(+)
net/ipv4/cipso_ipv4.c +0 -0
index bfaf327e9d12..e0480c6cebaa 100644
net/ipv4/cipso_ipv4.c void cipso_v4_doi_free(struct cipso_v4_doi
Quoted 8 lines
net/ipv4/cipso_ipv4.c +1 -1
@@ -472,6 +472,7 @@ void cipso_v4_doi_free(struct cipso_v4_doi
 *doi_def) 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);
--
Hi Paval,
Quoted 5 lines
this patch is already merged by Paul. See [1] for more details.
[1]
https://lore.kernel.org/netdev/CAHC9VhQZVOmy7n14nTSRGHzwN-y=E_JTUP+NpRCgD8rJN5sOGA@mail.gmail.com/T/

Quoted 4 lines

Hi, Dongliang!
Thank you for the information. I'm wondering, can maintainer pick only 1 patch from series, or I should send v2?

I don't know. Maybe you can wait for the reply of the maintainers.

Quoted 5 lines

With regards,
Pavel Skripkin