Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1625900431.git.paskripkin@gmail.com> <53de0ccd1aa3fffa6bce2a2ae7a5ca07e0af6d3a.1625900431.git.paskripkin@gmail.com>
In-Reply-To: <53de0ccd1aa3fffa6bce2a2ae7a5ca07e0af6d3a.1625900431.git.paskripkin@gmail.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 12 Jul 2021 11:03:03 -0400
Message-ID: <CAHC9VhS-bhpsKAhFwvQzkfAupbffKcb4sR6EDa1kMgdurnOiEg@mail.gmail.com>
Subject: Re: [PATCH 1/2] net: cipso: fix warnings in netlbl_cipsov4_add_std
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     davem@davemloft.net, yoshfuji@linux-ipv6.org, dsahern@kernel.org,
        kuba@kernel.org, netdev@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        syzbot+cdd51ee2e6b0b2e18c0d@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jul 10, 2021 at 3:03 AM Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+cdd51ee2e6b0b2e18c0d@syzkaller.appspotmail.com
> Fixes: 96cb8e3313c7 ("[NetLabel]: CIPSOv4 and Unlabeled packet integration")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  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 <paul@paul-moore.com>

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").

> diff --git a/net/netlabel/netlabel_cipso_v4.c 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
> @@ -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

-- 
paul moore
www.paul-moore.com