Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Nov 2022 10:22:32 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Denis Arefev <arefev@swemel.ru>
Cc:     Simon Horman <simon.horman@netronome.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>, oss-drivers@netronome.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        ldv-project@linuxtesting.org, trufanov@swemel.ru, vfh@swemel.ru
Subject: Re: [PATCH] lag_conf: Added pointer check
Message-ID: <Y3NaWM91RLUKFrLg@corigine.com>
References: <20221115085637.72193-1-arefev@swemel.ru>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221115085637.72193-1-arefev@swemel.ru>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?H8Gk9H1hEu27g24NWx9Rl8KI/Anq08w/gDPDSgqE3dI+hjjd4WERlUJinPX9?=
 =?us-ascii?Q?eOV41fo7rsYlnoj9qTbNi38EooFpj5+KQEY95jjYVRrmqVwhko/DcdgEI0Sl?=
 =?us-ascii?Q?f1DvkGr2mh+uWqzrA9J17VzYxwvvPl850ij/d46XJdCdowQZCML8TaCYwac4?=
 =?us-ascii?Q?GTJKgzRZV0G99stkhAboYy1lC1qrRJJlXivCHTd2Fnl9P3iT9JrPCjqW2oxY?=
 =?us-ascii?Q?2r1Gys7CGoyPJ4z+aj8/J7TUwyR1w4/VQ+WEPcLvLeFyhjIAgR1JjvDwNS6O?=
 =?us-ascii?Q?u5lLD/6JA3s/buCjl5KX4ehAi0zbk5lITzMkZTwazElXDlXI+uerPwHE4691?=
 =?us-ascii?Q?ph30uaXkTNcX9R7OJhQK0Ik2cPXVCd2eNQG6GwEqvqUX7l/BdruDxVeZOMs6?=
 =?us-ascii?Q?mXbEh9B7KVNuaFY7VRyKypWRMdfEiEbv7An09I1m0T4W0AEzWT6o6l+JSx8E?=
 =?us-ascii?Q?eyDMq8rFWwVOcDR+rCXN48lanrJIpmqOv0o7usnL5FvTE2+T6BsaUxurU0ij?=
 =?us-ascii?Q?HlSjWDjd3QANulGhW2reGQzjIme/neM0wS9LK5Qevh5Qq3GzQNH/zGh6mzlx?=
 =?us-ascii?Q?IxdMmRUvC9bnEHGjacrKUn3SFllLrJjiMpUYcEHMVyHVlcag2wCwP2l7ekAp?=
 =?us-ascii?Q?qlVMpvcrq3nhNwIu7+okGx//bxQlDXTd946K0sqZVxU+4x0Dz686i1VGHNou?=
 =?us-ascii?Q?axkA9O+SfAVcGusMlS/kbB1P6Or/UqPR2Y+g67p9v8TL03wm3YbUvS1ETKhH?=
 =?us-ascii?Q?YCxWpcscXzksfMQsvVyPWHoqNW/SMUOZjmJd2YI89BO8JBk6WJLsnUd+vVF3?=
 =?us-ascii?Q?XvQMa9zpG24+r48Iwl7oiQKwFixlaEpSKnjRmFSKD4FbiL8A1rTpvnUocnOH?=
 =?us-ascii?Q?tem447rWc5x5HUA2mnZ9q6X/iySPMj9KyiqvWx7+ufoIgz2dkMhDjVddB2xl?=
 =?us-ascii?Q?e7fqKJ3LCBgTUjU+hcYJSTKaExRSB0WwTRgptSH24qmrc0h0dYC8lTzS73yw?=
 =?us-ascii?Q?1vuNpdBWu5X1/Nxv9Usqeuf3IQZEJGZXXP8C/U4ZvTfZKP/2tVpFjmZNSC3u?=
 =?us-ascii?Q?Vd83y9FgFVu6oTtExvEljF7V6VL7xvw2xQMZ+RbnHwIjc3MBLmHeEVrYQpzp?=
 =?us-ascii?Q?iPJPLISWqLPfbBc207bt4jLtZMeA0XxQYo++mTxrNRTs1Ef27o5QYNunyNl+?=
 =?us-ascii?Q?lccB1LzeVkHeD2lYcJIV8rEnOiUmpOWs0PmdksblUfq54EjgEDpvPtl4w0bx?=
 =?us-ascii?Q?oQ0tkjsNC6p0aDucSU747l2HYJZaRNIsP/q+tV0ymDDsknOmA8jQBPz110NJ?=
 =?us-ascii?Q?lwX17mcxDELgPS9DD8fWXQSeELgLN+0ZRvedhDPua6LQ02kMZ5VeZde56UFn?=
 =?us-ascii?Q?c/MqtcIGYVISQCHEod+juNdZ9AcIOYj4bdZLWuPBcgcROqpfsbXdcoDEyLwD?=
 =?us-ascii?Q?EpH/XupW9KgQjSe4+WEwmyZlE2zcz/tHL8Hphya2bQ8WL6pJZw1dd72AObx0?=
 =?us-ascii?Q?6suRAJ1NXLfwE50VgOpXdDc8vNLsgGdOKRx/Xh1+cTwHMNyCkmegEG4NPJ4U?=
 =?us-ascii?Q?gE3OQ7nDO2uK+nfIHYGW4p5Ml6eXZd5ijgaXA1HY/bN6o/ZJ0xEchitg+CRZ?=
 =?us-ascii?Q?Dt54LPVOGqe/NLjN76eYQM8J8Q4nwk/lVBzaAPgr8NFa4nM2FOyL+MAC3LBa?=
 =?us-ascii?Q?NtvyEQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cc1a5ca3-ef85-4902-06a9-08dac6eaf087
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2022 09:22:40.1059
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iGo6eQeNhYhOsXourtScdmAc6xwwMdYyhCuIOJD6Zq8vHEwaQo6Sp9xcxjT1djiNh01G6TCiYXESRUwFdWkslXexrlrg/k8aEi327kz9vaw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR13MB5854
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 15, 2022 at 11:56:37AM +0300, Denis Arefev wrote:
> [You don't often get email from arefev@swemel.ru. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Return value of a function 'kmalloc_array' is dereferenced at lag_conf.c:347
> without checking for null, but it is usually checked for this function.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Denis Arefev <arefev@swemel.ru>

Hi Denis,

thanks for highlighting this problem.

> ---
>  drivers/net/ethernet/netronome/nfp/flower/lag_conf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/lag_conf.c b/drivers/net/ethernet/netronome/nfp/flower/lag_conf.c
> index 63907aeb3884..95ba6e92197d 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/lag_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/lag_conf.c
> @@ -276,7 +276,7 @@ static void nfp_fl_lag_do_work(struct work_struct *work)
> 
>         mutex_lock(&lag->lock);
>         list_for_each_entry_safe(entry, storage, &lag->group_list, list) {
> -               struct net_device *iter_netdev, **acti_netdevs;
> +               struct net_device *iter_netdev, **acti_netdevs = NULL;

I don't think it's necessary to set acti_netdevs here as
it is always set before use by the call to kmalloc_array().

>                 struct nfp_flower_repr_priv *repr_priv;
>                 int active_count = 0, slaves = 0;
>                 struct nfp_repr *repr;
> @@ -308,6 +308,8 @@ static void nfp_fl_lag_do_work(struct work_struct *work)
> 
>                 acti_netdevs = kmalloc_array(entry->slave_cnt,
>                                              sizeof(*acti_netdevs), GFP_KERNEL);
> +               if (!acti_netdevs)
> +                break;

The indentation here doesn't look right.


Regarding the problem at hand, yes, I agree that it seems
that kmalloc_array() should be checked. But I am concerned that
simply break'ing here may lead to a bad state. And I'd like to ask
for some time to examine this more closely.

>                 /* Include sanity check in the loop. It may be that a bond has
>                  * changed between processing the last notification and the
> --
> 2.25.1
>