Message-ID: <90adc80b-15d4-0938-e05c-a231991f5b77@intel.com>
Date:   Fri, 17 Feb 2023 14:46:56 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] net-bridge: fix unsafe dereference of potential null ptr
 in __vlan_del()
Content-Language: en-US
To:     Alexander Sapozhnikov <alsp705@gmail.com>
CC:     Roopa Prabhu <roopa@nvidia.com>,
        Nikolay Aleksandrov <razor@blackwall.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        <bridge@lists.linux-foundation.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <lvc-project@linuxtesting.org>
References: <20230217131657.12649-1-alsp705@gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230217131657.12649-1-alsp705@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MkphZ1JaVDUxL1F4WVJydVA2WjM5YnZndGkrOTZyc2s5dzJOQlJQWDZwaDdW?=
 =?utf-8?B?dGs3YU43bE5sNVdGZEJvUzJKelFtdXlOY0RONGpZTGswcGRDTnE4SVpYdHBw?=
 =?utf-8?B?TlVUVllOMThTWmJnYzZvaTdEWlI0QzRKWFk1T21vdHl6UkE1cG9zZ0pLM1ZQ?=
 =?utf-8?B?UGxvbVZmYS85WkFlWU92aUM2WjJmdjBlQUxiUkxOTGsrclc4aGVlUFo3blY2?=
 =?utf-8?B?WlpGb1dlcEdYTDZXL1RuNUhyOFVOM3Y4aHpNaHhsQXBZclYzWTNFbnZncUxY?=
 =?utf-8?B?L3lSSEo3YzAyeHpXNXhnbWE3UW83YWFGWXZEQlJTZERVK2pTaDc3a1lEVlg0?=
 =?utf-8?B?cFU2OEM0SjZQUVJFU2xxTG1PN0FGTmhoMGFCS3pBTGRBd0NFTS9URXZCRk9Q?=
 =?utf-8?B?Z0JKQWd5SDNudjUwRm13czA1blZqMDRocVRWZHcrSVNqOHMyTktOVXJPdzJD?=
 =?utf-8?B?Z0hjeVRYejdQN0tuQXJzKy9icWI5L1BNSjRYVC9QZFpLQXh6ZE9IY2QvUWs3?=
 =?utf-8?B?ZW1oT0ZZTjM2VDRLN2ZLN1ZmdTlKM1VTZ1A1eXRBVklrOGRHbW56WTZ4Mnl0?=
 =?utf-8?B?STh6ampPNGswOHN0cWNwOG5XUkdKV1dTSTVqOVBFR0JnbHJsUHA5L3c4RTdS?=
 =?utf-8?B?OFRJZUxBQW42Qm9lNDVFaGJtaE53MS82cHhNTm9Hdk5qVEJ4dkYwQnJueVhO?=
 =?utf-8?B?YVZDbGNZeVdNbjE0bFU3YlV0SXp2elRTQndaKzZqVCs0QlVWeEZ1QTkyYzVx?=
 =?utf-8?B?amlIaWw5cDNSeHVkNHdSRGZQVTc2TWtCbGlTU0Y3NHRyb3VwZ05vaUVkUmpB?=
 =?utf-8?B?K0plaW9BQjlyOEdqeTdJQVRndWJtMU5QRWdDVjRlKzN2WjFuVVY2QlhXaEpB?=
 =?utf-8?B?dGNDRVRLTFVxZUkrMkdjVG5IZFR2NGtlaFpieVBzRGNVWStMTUFRc3JiSkky?=
 =?utf-8?B?LzVOalBNUXM2WGJ3Vms1c3RIVVROMFZBLzBCRCt1RE8wYUEyaHozbG1aWm4w?=
 =?utf-8?B?bG96RGVRMXU5TkhHUThnZWtqSWFVNkZpN29DYitDRDdXMnNZWXRRVEFZNHlO?=
 =?utf-8?B?SklxOGVWN3A3V2JLM3pVb3dTRWlsbXBWb1NBT0UvKzdKZWxVWmxsMTA4ZTZw?=
 =?utf-8?B?Q1p6MjhGY3JSaVBKUkpWY3QyaXlDMmg0eVJxOUxGbXZJWmcyTXhJM3RzdTZm?=
 =?utf-8?B?YktuWWJEdnFJcnA3Wkc1YnU3bUM0SlM3bFgraGwySThUT0RHeUZFZnRjdlpt?=
 =?utf-8?B?S1RzL1lxVFFDOVN6cjMwV0VxUm5USWZBR0JTRFpaeU01WkJDWUIwZWNnNGgz?=
 =?utf-8?B?T1RScnZvelFJdlE2N1BPanhrSlFSRSsxRkpjbmdWWmxIMGExQ1ZCbHYvdzZm?=
 =?utf-8?B?UEhDekFyZHBQTTdldGhyUEFLZkNCay9GUXVtZVhJWnQ2V1lBS2dLaFZ1MUph?=
 =?utf-8?B?M0pHKzErdzRTcE9lZnZTTElyOFZMZUx6aVBSRnpXZDB3Q2daSEUxTFZ0cTMz?=
 =?utf-8?B?VURTYXlaVHJlN3g3YWZQWUdnZ2RSaVNsYko0NnJmazk3RUxkSWRqd1IzcjI1?=
 =?utf-8?B?TGxQWFg3Wmo0OGVTSWV6c0hZR1ZFNEpYV0RlSkhSeDhwbHZCdjNrZWNTeURh?=
 =?utf-8?B?dEJ1c2lCSnZncWpRazVCY29zQTJxSDh2V3lJL1c5YUFSK1dJTGkxMXZ1ZlZn?=
 =?utf-8?B?bDFWejdiakdvVE1KY1N6UGZFWlpJaUFDOTBkQkVVQ3Bqem9Dc0pDRmdhYUJB?=
 =?utf-8?B?azF6aGhJa1BiQ2pMNU0yMTA3b1Y0c2RIQ1FYSXZzWDMyV1QwcVhBZ2NLSGEx?=
 =?utf-8?B?d2NNTWJYczczWExpVGY5UXN4L0RiK2lUaTExVWc1TXBYTWFmWHoxSThiTW1Y?=
 =?utf-8?B?QXhLaXFpN1FzYkVCSWlxcUt1cXJiSG94K1NHa3RPTk9ZbXVFUHFmUEFwK05T?=
 =?utf-8?B?VUVXaFAyN1RUVDArRVd1am9QdWx4c21SVDBoL3kwd2FvY3pPaXNIMHZnQnpW?=
 =?utf-8?B?UVhZQ0krL29wU2M1S2Zic3cxUnVzdm1lcUlUcTV0a3RWK2ovZE5uUC95dHNO?=
 =?utf-8?B?a1RISVI5RHRqZnhoOXVka0tudGVqTlg4bXY2RTluUm5PNHo1UE5WOFVUUHhk?=
 =?utf-8?B?S1FpVFpMZC9LdEFBNS9wWVZPWkx5TjRlOXA5N2hlQm1CSU96ZjAvVTB0SXhl?=
 =?utf-8?Q?/LvBLfeuUFcS35M5YOuiGvA=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 48bb6c47-a171-4e97-88db-08db10eda990
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2023 13:48:34.3047
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: siJF76Kj2+ncTNrqpl0ViysXKJj5/gW30VZhVA3th8+8l/6uxoYFCPjtpfz4AZdGoXnwO9p7xcyrlCxcz25QlWoaK4DiVmYpZmSkb9Ip1xQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7828
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Alexander Sapozhnikov <alsp705@gmail.com>
Date: Fri, 17 Feb 2023 16:16:57 +0300

> [PATCH] net-bridge: fix unsafe dereference of potential null ptr in
> __vlan_del()

Must be:

[PATCH net] net: bridge: fix unsafe dereference of potential null ptr in
__vlan_del()

> After having been compared to NULL value at br_vlan.c:399,
> pointer 'p' is passed as 1st parameter in call to function
> 'nbp_vlan_set_vlan_dev_state' at br_vlan.c:420, 
> where it is dereferenced at br_vlan.c:1722.

Do you have a reproducer for this or it's purely hypothetical? I'm not
saying it's strongly required, at least I wouldn't require it, just
curious. And sometimes without a stable repro you should target
net-next, not net, but better ask the bridge maintainers re this.

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 

Here you should have a "Fixes:" tag with a reference to the commit which
introduced this nullptr-deref.

> Signed-off-by: Alexander Sapozhnikov <alsp705@gmail.com>

You have 2 copies of this patch in the list. The first one lacks proper
commit message, this one has it. But you didn't marked this resent as v2
(it's v2 in fact).
Please mark it accordingly next spin and don't forget always provide a
changelog between versions.

> ---
>  net/bridge/br_vlan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index bc75fa1e4666..87091e270adf 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -417,7 +417,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>  				       br_vlan_rht_params);
>  		__vlan_del_list(v);
> -		nbp_vlan_set_vlan_dev_state(p, v->vid);
> +		if (p)
> +			nbp_vlan_set_vlan_dev_state(p, v->vid);
>  		br_multicast_toggle_one_vlan(v, false);
>  		br_multicast_port_ctx_deinit(&v->port_mcast_ctx);
>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);

Thanks,
Olek