linux-kernel - RE: [PATCH v3] Introduce support for Systems Management Driver over WMI for Dell Systems

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] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 21 Sep 2020 18:23:11 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        mark gross <mgross@...ux.intel.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: RE: [PATCH v3] Introduce support for Systems Management Driver over
 WMI for Dell Systems

> 
> > +
> > +		"integer"-type specific properties:
> > +
> > +		min_value:	A file that can be read to obtain the lower
> > +		bound value of the <attr>
> > +
> > +		max_value:	A file that can be read to obtain the upper
> > +		bound value of the <attr>
> > +
> > +		scalar_increment:	A file that can be read to obtain the
> > +		resolution of the incremental value this attribute accepts.
> > +
> > +		"string"-type specific properties:
> > +
> > +		max_length:	A file that can be read to obtain the maximum
> > +		length value of the <attr>
> > +
> > +		min_length:	A file that can be read to obtain the minimum
> > +		length value of the <attr>
> > +
> > +What:		/sys/devices/platform/dell-wmi-sysman/passwords/
> > +Date:		December 2020
> > +KernelVersion:	5.10
> > +Contact:	Divya Bharathi <Divya.Bharathi@...l.com>,
> > +		Mario Limonciello <mario.limonciello@...l.com>,
> > +		Prasanth KSR <prasanth.ksr@...l.com>
> > +
> > +		A BIOS Admin password and System Password can be set, reset or
> > +		cleared using these attributes. An "Admin" password is used for
> > +		preventing modification to the BIOS settings. A "System" password
> is
> > +		required to boot a machine.
> > +
> > +		is_password_set:	A file that can be read
> > +		to obtain flag to see if a password is set on <attr>
> > +
> > +		max_password_length:	A file that can be read to obtain the
> > +		maximum length of the Password
> > +
> > +		min_password_length:	A file that can be read to obtain the
> > +		minimum length of the Password
> > +
> > +		current_password: A write only value used for privileged access
> > +		such as setting attributes when a system or admin password is set
> > +		or resetting to a new password
> > +
> > +		new_password: A write only value that when used in tandem with
> > +		current_password will reset a system or admin password.
> > +
> > +		Note, password management is session specific. If Admin/System
> > +		password is set, same password must be writen into current_password
> > +		file (requied for pasword-validation) and must be cleared once the
> > +		session	is over. For example,
> > +			echo "password" > current_password
> > +			echo "disabled" > TouchScreen/current_value
> > +			echo "" > current_password
> 
> So I know this was mentioned before already but one concern I have here
> is that there is a race where other users with write access to say
> TouchScreen/current_value
> may change it between the setting and the clearing of the current_password
> even if
> they don't have the password.

I don't think there is much that can be done here other than move to something atomic
like sending the password as part of the request.

echo "foobar123|enabled" | sudo tee /sys/devices/platform/dell-wmi-sysman/

That isn't really pretty - and worse you can no longer have a process fetching
authentication from escrow that is different from your "setter" process.

> 
> This is esp. relevant with containers. I'm not aware about all the intrinsics
> of
> sysfs and containers, at a minimum we need to check if it is possible to
> disallow
> all writes to the attributes when sysfs is mounted inside a container (so
> outside of the
> main filesystem namespace).

Containers by default mount sysfs as read only unless you use the '--privileged'
flag.

https://www.redhat.com/sysadmin/privileged-flag-container-engines

> 
> If someone is reading along who happen to knows this, please enlighten me :)
> 
> > +
> > +
> > +What:		/sys/devices/platform/dell-wmi-sysman/attributes/reset_bios
> > +Date:		December 2020
> > +KernelVersion:	5.10
> > +Contact:	Divya Bharathi <Divya.Bharathi@...l.com>,
> > +		Mario Limonciello <mario.limonciello@...l.com>,
> > +		Prasanth KSR <prasanth.ksr@...l.com>
> > +Description:
> > +		This attribute can be used to reset the BIOS Configuration.
> > +		Specifically, it tells which type of reset BIOS configuration is
> being
> > +		requested on the host.
> > +
> > +		Reading from it returns a list of supported options encoded as:
> > +
> > +			'builtinsafe' (Built in safe configuration profile)
> > +			'lastknowngood' (Last known good saved configuration profile)
> > +			'factory' (Default factory settings configuration profile)
> > +			'custom' (Custom saved configuration profile)
> > +
> > +		The currently selected option is printed in square brackets as
> > +		shown below:
> > +
> > +		# echo "factory" > sys/devices/platform/dell-wmi-
> sysman/attributes/reset_bios
> > +
> > +		# cat sys/devices/platform/dell-wmi-sysman/attributes/reset_bios
> > +		# builtinsafe lastknowngood [factory] custom
> > +
> > +		Note that any changes to this attribute requires a reboot
> > +		for changes to take effect.
> > +
> > +What:		/sys/devices/platform/dell-wmi-
> sysman/attributes/pending_reboot
> > +Date:		December 2020
> > +KernelVersion:	5.10
> > +Contact:	Divya Bharathi <Divya.Bharathi@...l.com>,
> > +		Mario Limonciello <mario.limonciello@...l.com>,
> > +		Prasanth KSR <prasanth.ksr@...l.com>
> > +Description:
> > +		A read-only attribute reads 1 if a reboot is necessary to apply
> > +		pending BIOS attribute changes.
> > +
> > +			0:	All BIOS attributes setting are current
> > +			1:	A reboot is necessary to get pending BIOS attribute
> changes
> > +				applied
> > +
> > +
> > +		Note, userspace applications need to follow below steps for
> efficient
> > +		BIOS management,
> > +		1.	Check if admin password is set. If yes, follow session method
> for
> > +			password management as briefed under password section above.
> > +		2.	Before setting any attribute, check if it has any modifiers
> > +			or value_modifiers. If yes, incorporate them and then modify
> > +			attribute.
> 
> OK, so as also mentioned in the v1 thread, I would like to see the uevent
> disappear
> since it is somewhat of a heavy mechanism and not necessary when userspace
> specifically
> cares about a single sysfs attribute changing.
> 
> Instead we can allow userspace to use poll() for POLL_PRI on this file to be
> notified
> when it goes from 0 -> 1. See the v1 thread for some example code how to wake
> the
> poll() in this case.
> 
> Regards,
> 
> Hans