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;
Message-ID: <30e4bf50-7950-0b3c-67b5-6028b7114da2@intel.com>
Date:   Wed, 12 Apr 2023 15:34:51 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [RFC PATCH net] sfc: Fix use-after-free due to selftest_work
Content-Language: en-US
To:     Ding Hui <dinghui@sangfor.com.cn>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <ecree.xilinx@gmail.com>, <habetsm.xilinx@gmail.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <pengdonglin@sangfor.com.cn>, <huangcun@sangfor.com.cn>
References: <20230412005013.30456-1-dinghui@sangfor.com.cn>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230412005013.30456-1-dinghui@sangfor.com.cn>
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?L0FDT2hpa2lyTVcrQzFiQmI5UG9rTDY5M0NSTnl6ZUxiMjY4NmhtWEg3WCsz?=
 =?utf-8?B?c0xCTzhTQXYxVUtKNG1mOThXMHRkMnFVYnNxNG9sRVJwV1dGYnQwQ1hWTUdl?=
 =?utf-8?B?ejRHOWhHQ3l3ZzhabDR1VVR6NXdMOUJyTGhvUWs3eldyeTdHdDFKT0dZR0hp?=
 =?utf-8?B?TlErd0o0VTV0M1ZNL0xRaEQ0UTgyUm9CUm1FMUQwcGdTMVdrWjM2azVYZDRs?=
 =?utf-8?B?Q0NQaXcyb2JyUkVKSnNvaHhEK2FnaGhiTTVLUk5FZ3NnQk1CTFJlZTM3UjlJ?=
 =?utf-8?B?MkY5NnVVc2wrbnc4WGYxUTR0a2czdnFVK2N0cHpKYms3MmI4Si9uU0VIS2xt?=
 =?utf-8?B?MDJkcUhtWjF5TzJJeXJYdklJcGNuUUNXS2lLdi84cHZiOWJHLy9JMU5RcHhw?=
 =?utf-8?B?YTU0U3NQaG0zSU1qRFppSjBJUU1ETTdLYnJneUZ5ZDgxMmxSR0NxN3FTK0VT?=
 =?utf-8?B?QTZ1Rkk0UVFDOGc2dElWbmNvc1dHWWZaRS9KdlBZU1JRRmJFcThhOWpxdy85?=
 =?utf-8?B?MDFKOHFHZ1pHYXkveklFc1ZDNnFtY2VUUVFYeDU0SVhaR0Z3NmpLWGJiY2Zy?=
 =?utf-8?B?WkpHQnlCemRqUDh3U3pWWmd2dkpLeFRUdTR1OGpScUwrZnVnQjNyRXE0Rk50?=
 =?utf-8?B?R1ZLdnJXejIxN1RXYWpibS9uL1ZZbWZqdEFQcGNVVElMRTZ2Vjd3ZU9FdWdM?=
 =?utf-8?B?UENma2I4bTRLaWgzMzJ3MjFobGJZWnhlY3lVUjluQVA0bHJEVmlzbG9ZOU5L?=
 =?utf-8?B?MXBEdExBVGdqeUw3Zk9qL0JVTUpyNkxpdVZrT2hzMFNkcmhMWDI2aERCVnYy?=
 =?utf-8?B?am9UdGVpVzRLUzc3VUMrYUpFM3JvRjkwanVJZGpDYXdlelJMWU5vUHpsNjFn?=
 =?utf-8?B?L2RPWEtvUTZEcmNxTER2NUJsZndobXduT1cvV2VWenBHL1B2NUN6eTBRdGcw?=
 =?utf-8?B?eXFNWUoySmlHaU1nbVczZWF6TWZ6cmV6VGV6WVRRNk9OaHcyZ2NYZndNNDZ6?=
 =?utf-8?B?NzNieHYycFcwdXpERWM2c3FzQm5HUmljeXdUaFpQL1dMVjZVZEZKdFZYUjJ2?=
 =?utf-8?B?Qmoyaks1V0E3Q01pajBQcE9LbUgyVVlWQzFrZDBVM1Zod2RtSi9QTko4Nzlh?=
 =?utf-8?B?WW9VQkhJaXVIS2VVaXNEa3BtZmZTdzVDSlloWWZ3UDNzeWF3cUs0SHA3Vk5h?=
 =?utf-8?B?djVRc2RoSE5lb2IvVHpJMlA3NUx6UlpyWlR1c3hLcElSL2Y1WXkvMmpGc2pk?=
 =?utf-8?B?eHZVeXNNL05iWUk4enRkOG9PL05DbGR6N0ZrWERQOC94YmlTM0d0a0lZbWl3?=
 =?utf-8?B?ZCtWdU5xRG0wbDFCcHd2UHZvL1MvNEg5Y1V1czc0SER5a0RBMzg4bmFRSVE1?=
 =?utf-8?B?N29lajg4Q2lOd0ViZmprRGxEWEIvb25rejUvUGZKMjl1WjZJNDFrWlF4ZG51?=
 =?utf-8?B?eGRNS3pZOGlDbzV4TFY0Ykc1MXlQQ2pRZ0RTckhCdzc0M2xZNHJDVCs5U213?=
 =?utf-8?B?bXNwNHBaM3Y3ZjNZbE96bGZBWmFkMnNwWk9OWVJyRU00dmxwRGJqRnB2UUlF?=
 =?utf-8?B?OFFJV2w1S1dGZi9aanpMdlpWemlFeUdRNHRFdzlDTm1xY3I5RFZ0M2F3QUtK?=
 =?utf-8?B?SU8xT3ZIU3l0L2tEcUFxK2hEaXBBd2tlc0JNbEhidXpnNS9GT2x4VUErdGtU?=
 =?utf-8?B?ejhWMm9uWUxWZHF0aTI5dlRmMDh6SFdDVTVXeVpPYXVLQ2prVGp1dXdCamhF?=
 =?utf-8?B?RkhDc2JhM1hUM3djUThHQ2FQMTEranBaQ2hoM3hHMHM1eG9UUUM0ZnloL25L?=
 =?utf-8?B?M2J0aWNxOUZuNUk0bXJPUHAvZ21ZM2FYTzhLeStqOXJYNnFJWHdFUUQwNnQ0?=
 =?utf-8?B?WCt3NXl4WWlIamh1b003TmNqMVp5YlhJeE1HRFlRcDM3dGU2Tmg4OElhUjZa?=
 =?utf-8?B?SUQyTEFGNlhuUDVDYWxwa0dNalFtVE1yU3pndmFEV2ZOZlJpbGNVOGlFMi9L?=
 =?utf-8?B?Y3BuMit4a3dWTjRIbytyNmlSTVVRaUx1NS9Ra2l1WWc5RDliQm56NDVBS2ht?=
 =?utf-8?B?cEdTVGtsTmR6b0RhS0RQbW5DL1lNUTdNK3B3NmlPZ2ZBN1FOdG9ZWWxCa0dW?=
 =?utf-8?B?cE5oYmhXcmlOMG9Xc3pabnozTk5SWWZwdkFxWkl4WEFjd3JDejhsUGZMeFh2?=
 =?utf-8?B?S0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6eb684cd-844c-468e-fca2-08db3ba61e98
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2023 22:34:46.9037
 (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: MsugZ5T3JRAmhC2xXegQm0xDdlOXc5sO/suL64uqQg4lIgzqNQEAPn+dLo+zYZNezIcXmWBiRLCAaJO3qfaVdPokAU9ehsgo2VWx0WiYWCs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4816
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,
        URIBL_BLOCKED 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 4/11/2023 5:50 PM, Ding Hui wrote:
> There is a use-after-free scenario that is:
> 
> When netif_running() is false, user set mac address or vlan tag to VF,
> the xxx_set_vf_mac() or xxx_set_vf_vlan() will invoke efx_net_stop()
> and efx_net_open(), since netif_running() is false, the port will not
> start and keep port_enabled false, but selftest_worker is scheduled
> in efx_net_open().
> 
> If we remove the device before selftest_worker run, the efx is freed,
> then we will get a UAF in run_timer_softirq() like this:
> 
> [ 1178.907941] ==================================================================
> [ 1178.907948] BUG: KASAN: use-after-free in run_timer_softirq+0xdea/0xe90
> [ 1178.907950] Write of size 8 at addr ff11001f449cdc80 by task swapper/47/0
> [ 1178.907950]
> [ 1178.907953] CPU: 47 PID: 0 Comm: swapper/47 Kdump: loaded Tainted: G           O     --------- -t - 4.18.0 #1
> [ 1178.907954] Hardware name: SANGFOR X620G40/WI2HG-208T1061A, BIOS SPYH051032-U01 04/01/2022
> [ 1178.907955] Call Trace:
> [ 1178.907956]  <IRQ>
> [ 1178.907960]  dump_stack+0x71/0xab
> [ 1178.907963]  print_address_description+0x6b/0x290
> [ 1178.907965]  ? run_timer_softirq+0xdea/0xe90
> [ 1178.907967]  kasan_report+0x14a/0x2b0
> [ 1178.907968]  run_timer_softirq+0xdea/0xe90
> [ 1178.907971]  ? init_timer_key+0x170/0x170
> [ 1178.907973]  ? hrtimer_cancel+0x20/0x20
> [ 1178.907976]  ? sched_clock+0x5/0x10
> [ 1178.907978]  ? sched_clock_cpu+0x18/0x170
> [ 1178.907981]  __do_softirq+0x1c8/0x5fa
> [ 1178.907985]  irq_exit+0x213/0x240
> [ 1178.907987]  smp_apic_timer_interrupt+0xd0/0x330
> [ 1178.907989]  apic_timer_interrupt+0xf/0x20
> [ 1178.907990]  </IRQ>
> [ 1178.907991] RIP: 0010:mwait_idle+0xae/0x370
> 
> I am thinking about several ways to fix the issue:
> 
> [1] In this RFC, I cancel the selftest_worker unconditionally in
> efx_pci_remove().
> 
> [2] Add a test condition, only invoke efx_selftest_async_start() when
> efx->port_enabled is true in efx_net_open().
> 
> [3] Move invoking efx_selftest_async_start() from efx_net_open() to
> efx_start_all() or efx_start_port(), that matching cancel action in
> efx_stop_port().
> 
> [4] However, I also notice that in efx_ef10_set_mac_address(), the
> efx_net_open() depends on original port_enabled, but others are not,
> if we change all efx_net_open() depends on old state like
> efx_ef10_set_mac_address() does, the UAF can also be fixed in theory.
> 
> But I'm not sure which is better, is there any suggestions? Thanks.
> 

I think this fix makes the most sense to me.

> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
> ---

net patches need a Fixes tag indicating what commit this fixes. This
being RFC is likely why that was left off?

>  drivers/net/ethernet/sfc/efx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> index 884d8d168862..dd0b2363eed1 100644
> --- a/drivers/net/ethernet/sfc/efx.c
> +++ b/drivers/net/ethernet/sfc/efx.c
> @@ -876,6 +876,8 @@ static void efx_pci_remove(struct pci_dev *pci_dev)
>  	efx->state = STATE_UNINIT;
>  	rtnl_unlock();
>  
> +	efx_selftest_async_cancel(efx);
> +
>  	if (efx->type->sriov_fini)
>  		efx->type->sriov_fini(efx);
>