netdev - Re: net/mlx5: Refactor tc flow attributes structure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 28 Sep 2020 16:25:46 -0700
From:   Saeed Mahameed <saeed@...nel.org>
To:     Colin Ian King <colin.king@...onical.com>,
        Ariel Levkovich <lariel@...lanox.com>,
        Roi Dayan <roid@...lanox.com>, Vlad Buslov <vladbu@...dia.com>,
        Saeed Mahameed <saeedm@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        linux-rdma@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: net/mlx5: Refactor tc flow attributes structure

On Mon, 2020-09-28 at 17:06 +0100, Colin Ian King wrote:
> Hi,
> 
> static analysis with Coverity has found a null pointer dereference
> issue
> with the following commit:
> 
> commit c620b772152b8274031083bdb2e11c963e596c5c
> Author: Ariel Levkovich <lariel@...lanox.com>
> Date:   Thu Apr 30 05:54:08 2020 +0300
> 
>     net/mlx5: Refactor tc flow attributes structure
> 
> The analysis is as follows:
> 
> 1240        slow_attr =
> mlx5_alloc_flow_attr(MLX5_FLOW_NAMESPACE_FDB);
> 
>     1. Condition !slow_attr, taking true branch.
>     2. var_compare_op: Comparing slow_attr to null implies that
> slow_attr might be null.
> 
> 1241        if (!slow_attr)
> 1242                mlx5_core_warn(flow->priv->mdev, "Unable to
> unoffload slow path rule\n");
> 1243
> 1244        memcpy(slow_attr, flow->attr, ESW_FLOW_ATTR_SZ);
> 
> Dereference after null check (FORWARD_NULL)
>     3. var_deref_op: Dereferencing null pointer slow_attr.
> 
> 1245        slow_attr->action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> 1246        slow_attr->esw_attr->split_count = 0;
> 1247        slow_attr->flags |= MLX5_ESW_ATTR_FLAG_SLOW_PATH;
> 1248        mlx5e_tc_unoffload_fdb_rules(esw, flow, slow_attr);
> 1249        flow_flag_clear(flow, SLOW);
> 1250        kfree(slow_attr);
> 
> there is a !slow_attr check but if it slow_attr is null the code then
> dereferences it multiple times afterwards.
> 
> Colin

Thanks Colin for the Report,

Ariel is handling this internally and we will be posting the patch
soon.