Re: [PATCH v3] Introduce support for Systems Management Driver over WMI for Dell Systems

[Hans de Goede]

Hi,

On 9/21/20 8:23 PM, Limonciello, Mario wrote:
> > > +
> > > +		"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@xxxxxxxx>,
> > > +		Mario Limonciello <mario.limonciello@xxxxxxxx>,
> > > +		Prasanth KSR <prasanth.ksr@xxxxxxxx>
> > > +
> > > +		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.

Right, I'm not saying this scheme is bad per se I just wanted to make
sure that this was brought up and discussed.

> 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.

Note you will likely need some form of IPC, say dbus to your escrow process
anyways to ask it to unlock. So you could just change the "unlock" request
to a "set Camera=Enabled" request and have the process which has access
to the authentication token make the changes itself.

> > 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

Thanks that is good to know and does address most of my concerns. What I was
wondering about is if we can enforce this in the kernel even for --privileged
containers.  I guess another question would be if we can do that, should we?

Regards,

Hans