linux-kernel - Re: [PATCH] ufs: crypto: add host_sem lock in ufshcd_program

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]

Message-ID: <a0da2dc5-80e5-4fd7-92a0-69c399f2a171@acm.org>
Date: Fri, 21 Mar 2025 09:53:31 -0700
From: Bart Van Assche <bvanassche@....org>
To: ZhangHui <zhanghui31@...omi.com>, ebiggers@...nel.org
Cc: James.Bottomley@...senpartnership.com, alim.akhtar@...sung.com,
 avri.altman@....com, linux-kernel@...r.kernel.org,
 linux-scsi@...r.kernel.org, martin.petersen@...cle.com,
 peter.griffin@...aro.org
Subject: Re: [PATCH] ufs: crypto: add host_sem lock in ufshcd_program_key

On 3/21/25 12:45 AM, ZhangHui wrote:
> I have checked the device_shutdown process and it seems only wait
> for the resume that has not been processed to be completed, and
 > then continue. It does not seem to cause pm_runtime_get_sync to return
 > an error.

device_shutdown() is a kernel function. File systems must be unmounted
by user space code before the device_shutdown() kernel function is
called. The sequence followed by systemd is as follows (see also the
systemd source file src/shutdown/shutdown.c):
* Call sync().
* Send SIGTERM and SIGKILL to all running processes.
* Unmount all filesystems, deactivate swap devices, detach loopback
   devices, stop md devices and detach dm devices.
* Call sync() again.
* Call the reboot() system call.

 From kernel/reboot.c:

SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
		void __user *, arg)
{
	...
	case LINUX_REBOOT_CMD_POWER_OFF:
		kernel_power_off();
		do_exit(0);
		break;
	...
}

void kernel_power_off(void)
{
	kernel_shutdown_prepare(SYSTEM_POWER_OFF);
	if (pm_power_off_prepare)
		pm_power_off_prepare();
	migrate_to_reboot_cpu();
	syscore_shutdown();
	pr_emerg("Power down\n");
	kmsg_dump(KMSG_DUMP_POWEROFF);
	machine_power_off();
}

static void kernel_shutdown_prepare(enum system_states state)
{
	blocking_notifier_call_chain(&amp;reboot_notifier_list,
		(state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
	system_state = state;
	usermodehelper_disable();
	device_shutdown();
}

>> Or does the UFS driver still need to check
 >> ufshcd_is_user_access_allowed() too? If that's the case, I'm also
 >> wondering whether it's okay to nest host_sem inside
 >> pm_runtime_get_sync().  Elsewhere in the UFS driver they are>> 
called in the opposite order.>
 > I found that ufshcd_is_user_access_allowed is used in many places in
 > the ufs driver code. What is the historical reason for this?

My understanding is that ufshcd_is_user_access_allowed() is only called
from sysfs and debugfs show and store callbacks. I'd like to remove that
function because my understanding is that access to sysfs and debugfs
attributes stops before the device .shutdown() callbacks are called.

Bart.