Date:   Wed, 25 Jan 2023 16:08:12 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Natalia Petrova <n.petrova@fintech.ru>
Cc:     Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Tony Nguyen <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org
Subject: Re: [PATCH] i40e: Add checking for null for nlmsg_find_attr()
Message-ID: <Y9FF3NfHYFdQlKei@corigine.com>
References: <20230125141328.8479-1-n.petrova@fintech.ru>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230125141328.8479-1-n.petrova@fintech.ru>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?cusIBQbpWy8SYVPwfI7/iWjf60ecQqPXkBTIDWHcsa2BmESHkugbRYjhMER/?=
 =?us-ascii?Q?kdSEmggf1U+RiPcgX7BzHV0ODFZ2nPdxR37zWTL/Hguenrl7JoBiKjzHLTmB?=
 =?us-ascii?Q?dQGW72clTvEMZ4gjC5BZD42Nw2k7flECi/ge1qTqguRPW75hoq7CgvhtSp0b?=
 =?us-ascii?Q?c8ieBs4xIxx7DJ13143MCtyQxshQef5wsqB4Hw51G3CdMUSgLc77JZY+Qc08?=
 =?us-ascii?Q?ZahmVSv6awt7C4Rtgf57xBQdZ7WCu8F7oDhL74HURTBOiQ7QjD83wfNAWS99?=
 =?us-ascii?Q?CCYhSCuRbEF7O24ZGBKCrg65cHARXtwKnsRGY0vGoCnEzmd6jiapMb7Y3u9R?=
 =?us-ascii?Q?HdJj4UOstC7j1LRxPqctMXKP3N7J1ULVKFJGwiac/PcKl9gSjpNgwQfIt/Uk?=
 =?us-ascii?Q?E3jmrsNzSNOKEefewO6ncI7wtrCrFOHXzeSpLJnGC6HD4yrGQ+aliJfg2vs5?=
 =?us-ascii?Q?opSY6NXX/pTv4q3icoMFRBziQGJBJRf7S6cQzL5Z8aSEPkXwCxLwWuTFMme3?=
 =?us-ascii?Q?3i6ABfyu91yZ4Q8CpGZJlzrBCMiFrJiGkfgcWvrHutChAnm6rvsBG0IKJW37?=
 =?us-ascii?Q?s7UNaAGT+T64ryTlURgSAkZiZ6UZhSIp9wHLou6uhBVXQB5azAAwPhVLKPYg?=
 =?us-ascii?Q?M7rFADJfgtn6DbmyXw4Rc/99Auw/Pgyp+6duAy3Cd47Fb8g5uqwvifvWc5jC?=
 =?us-ascii?Q?TZlKecR0I7o5xSj3CTSI5dKFoZ3X2Ydqvkwp7SkQUktShpm2Kc7w0zOCQXBU?=
 =?us-ascii?Q?cdRycxXRAeIagSsRVFHaUygGzBlNacxLLIGmwPPraZw3huWclFW3PTGecznK?=
 =?us-ascii?Q?ltx0xxRa5tU73iy1haGkD298yo2oWffjXDPW7FQsTRjxG9f2Gr7kkMcqK2Hm?=
 =?us-ascii?Q?LoO7weEYBLrdoHanWI3qzKruxaXeeCzE1CN/7DmHit0TEBLDISI0Uar7dXdZ?=
 =?us-ascii?Q?qMUEtOoeE160KJiPYSYmRo9n7jbDwJO7g4l/XNIuFl4lSbQF0JZpux6RpqPJ?=
 =?us-ascii?Q?G7M9DqcvZmS4TXN5UTHybhK8yuQ/QKJ7rl+y9I+UaN3pa6LweqBNw/xlAgal?=
 =?us-ascii?Q?ho84RHAH3KLr/RdcwnMn9jcYoT9QirlEcz1QILvm9lPmSLcrAqK9OKwP6tId?=
 =?us-ascii?Q?Xo3JP2/D/IyFaMS9aj8tKchafk8OOp/j2zLk7CeV9NgU34wtxjQ9bxI2KoFC?=
 =?us-ascii?Q?QIDHjDMsSyk2XeMcmIH/w9IHs68FZb/SsvO5j97Lki4EmZ0DSQooKxBqof4Y?=
 =?us-ascii?Q?LnofDgQAl0Nxe99EOBOtPL5UbSs503ABryHGZqnbqClGmijZ5lLIbzhVNAAX?=
 =?us-ascii?Q?iz5Nj/ane1sC4CywsSsqGfLPezbL2P3h1+KKEmFbzfw0ZJWiRuSt4a1NhHsX?=
 =?us-ascii?Q?fto8aXL/MW2F4t/Fy2Dy5Br02Ny+8DC+ClEPDUH6bY9DwH8opMUaLVWIgXph?=
 =?us-ascii?Q?Lql5cYhSVJOup0vL4QwbPwClk7Or3ErpESgqWujLS9MqpPGyVz5lk/7+NSTu?=
 =?us-ascii?Q?QABCcrG+a54AQrGx8mvbv6dcoayNU1mDCGLATzGEFPvY4iJt6wN+aEZZGbqh?=
 =?us-ascii?Q?pCaMOAc4J9YLT1nSJlcPnwrWfegtecuJufel0o4e9sLLqgCg3dsWHRlQMhzM?=
 =?us-ascii?Q?6VDrx0W+trGc7qkPUggO8hRBA11uqsGP4V6aFKizWg2+hYnErNYJJSjKbPuJ?=
 =?us-ascii?Q?6wfKfA=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 54e52dfb-8b0d-4bc6-a8ea-08dafee5fd5b
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2023 15:08:18.0745
 (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: LOWov/q9A3OvBxlbQ5mMExMsLAY7Xu+1/Zk+pckpffDVLs6rbhlVHkord7Hjg+AEENVtcLE/PsEsv21c0IsCJAxUw4czQofamA7y0gLZeNk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR13MB4461
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Jan 25, 2023 at 05:13:28PM +0300, Natalia Petrova wrote:
> The result of nlmsg_find_attr() 'br_spec' is dereferenced in
> nla_for_each_nested, but it can take null value in 'nla_find' finction,
> which will result in an error.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 51616018dd1b ("i40e: Add support for getlink, setlink ndo ops")
> Signed-off-by: Natalia Petrova <n.petrova@fintech.ru>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b36bf9c3e1e4..ed4be4ffeb09 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -13101,6 +13101,8 @@ static int i40e_ndo_bridge_setlink(struct net_device *dev,
>  	}
>  
>  	br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
> +	if (!br_spec)
> +		return -ENOENT;

Seems fine to me, but I wonder if -EINVAL is more appropriate.
nlh is invalid in this case.

>  
>  	nla_for_each_nested(attr, br_spec, rem) {
>  		__u16 mode;
> -- 
> 2.34.1
>