On 11/19/22 06:48, Ritesh Harjani (IBM) wrote:

Hello Nayna,

Hi Ritesh,

On 22/11/09 03:10PM, Nayna wrote:

quoted

On 11/9/22 08:46, Greg Kroah-Hartman wrote:

quoted

On Sun, Nov 06, 2022 at 04:07:42PM -0500, Nayna Jain wrote:

quoted

securityfs is meant for Linux security subsystems to expose policies/logs
or any other information. However, there are various firmware security
features which expose their variables for user management via the kernel.
There is currently no single place to expose these variables. Different
platforms use sysfs/platform specific filesystem(efivarfs)/securityfs
interface as they find it appropriate. Thus, there is a gap in kernel
interfaces to expose variables for security features.

Define a firmware security filesystem (fwsecurityfs) to be used by
security features enabled by the firmware. These variables are platform
specific. This filesystem provides platforms a way to implement their
   own underlying semantics by defining own inode and file operations.

Similar to securityfs, the firmware security filesystem is recommended
to be exposed on a well known mount point /sys/firmware/security.
Platforms can define their own directory or file structure under this path.

Example:

# mount -t fwsecurityfs fwsecurityfs /sys/firmware/security

Why not juset use securityfs in /sys/security/firmware/ instead?  Then
you don't have to create a new filesystem and convince userspace to
mount it in a specific location?

I am also curious to know on why not use securityfs, given the similarity
between the two. :)
More specifics on that below...

quoted

 From man 5 sysfs page:

/sys/firmware: This subdirectory contains interfaces for viewing and
manipulating firmware-specific objects and attributes.

/sys/kernel: This subdirectory contains various files and subdirectories
that provide information about the running kernel.

The security variables which are being exposed via fwsecurityfs are managed
by firmware, stored in firmware managed space and also often consumed by
firmware for enabling various security features.

That's ok. As I see it users of securityfs can define their own fileops
(like how you are doing in fwsecurityfs).
See securityfs_create_file() & securityfs_create_symlink(), can accept the fops
& iops. Except maybe securityfs_create_dir(), that could be since there might
not be a usecase for it. But do you also need it in your case is the question to
ask.

Please refer to the function plpks_secvars_init() in Patch 4/4.

quoted

 From git commit b67dbf9d4c1987c370fd18fdc4cf9d8aaea604c2, the purpose of
securityfs(/sys/kernel/security) is to provide a common place for all kernel
LSMs. The idea of

Which was then seperated out by commit,
da31894ed7b654e2 ("securityfs: do not depend on CONFIG_SECURITY").

securityfs now has a seperate CONFIG_SECURITYFS config option. In fact I was even
thinking of why shouldn't we move security/inode.c into fs/securityfs/inode.c .
fs/* is a common place for all filesystems. Users of securityfs can call it's
exported kernel APIs to create files/dirs/symlinks.

If we move security/inode.c to fs/security/inode.c, then...
...below call within securityfs_init() should be moved into some lsm sepecific
file.

#ifdef CONFIG_SECURITY
static struct dentry *lsm_dentry;
static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
			loff_t *ppos)
{
	return simple_read_from_buffer(buf, count, ppos, lsm_names,
		strlen(lsm_names));
}

static const struct file_operations lsm_ops = {
	.read = lsm_read,
	.llseek = generic_file_llseek,
};
#endif

securityfs_init()

#ifdef CONFIG_SECURITY
	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
						&lsm_ops);
#endif

So why not move it? Maybe others, can comment more on whether it's a good idea
to move security/inode.c into fs/security/inode.c?
This should then help others identify securityfs filesystem in fs/security/
for everyone to notice and utilize for their use?

quoted

fwsecurityfs(/sys/firmware/security) is to similarly provide a common place
for all firmware security objects.

/sys/firmware already exists. The patch now defines a new /security
directory in it for firmware security features. Using /sys/kernel/security
would mean scattering firmware objects in multiple places and confusing the
purpose of /sys/kernel and /sys/firmware.

We can also think of it this way that, all security related exports should
happen via /sys/kernel/security/. Then /sys/kernel/security/firmware/ becomes
the security related firmware exports.

If you see find /sys -iname firmware, I am sure you will find other firmware
specifics directories related to other specific subsystems
(e.g.
root@qemu:/home/qemu# find /sys -iname firmware
/sys/devices/ndbus0/nmem0/firmware
/sys/devices/ndbus0/firmware
/sys/firmware
)

But it could be, I am not an expert here, although I was thinking a good
Documentation might solve this problem.

Documentation on 
sysfs(https://man7.org/linux/man-pages/man5/sysfs.5.html) already 
differentiates /sys/firmware and /sys/kernel as I responded earlier.  
The objects we are exposing are firmware objects and not kernel objects.

quoted

Even though fwsecurityfs code is based on securityfs, since the two
filesystems expose different types of objects and have different
requirements, there are distinctions:

1. fwsecurityfs lets users create files in userspace, securityfs only allows
kernel subsystems to create files.

Sorry could you please elaborate how? both securityfs & fwsecurityfs
calls simple_fill_super() which uses the same inode (i_op) and inode file
operations (i_fop) from fs/libfs.c for their root inode. So how it is enabling
user (as in userspace) to create a file in this filesystem?

So am I missing anything?

The ability to let user(as in userspace) to create a file in a 
filesystem comes by allowing to define inode operations.

Please look at the implementation differences for functions 
xxx_create_dir() and xxx_create_dentry() of securityfs vs fwsecurityfs.  
Also refer to Patch 4/4 for use of fwsecurityfs_create_dir() where inode 
operations are defined.

quoted

2. firmware and kernel objects may have different requirements. For example,
consideration of namespacing. As per my understanding, namespacing is
applied to kernel resources and not firmware resources. That's why it makes
sense to add support for namespacing in securityfs, but we concluded that
fwsecurityfs currently doesn't need it. Another but similar example of it

It "currently" doesn't need it. But can it in future? Then why not go with
securityfs which has an additional namespacing feature available?
That's actually also the point of utilizing an existing FS which can get
features like this in future. As long as it doesn't affect the functionality
of your use case, we simply need not reject securityfs, no?

Thanks for your review and feedback. To summarize:

 From the perspective of our use case, we need to expose firmware 
security objects to userspace for management. Not all of the objects 
pre-exist and we would like to allow root to create them from userspace.

 From a unification perspective, I have considered a common location at 
/sys/firmware/security for managing any platform's security objects. And 
I've proposed a generic filesystem, which could be used by any platform 
to represent firmware security objects via /sys/firmware/security.

Here are some alternatives to generic filesystem in discussion:

1. Start with a platform-specific filesystem. If more platforms would 
like to use the approach, it can be made generic. We would still have a 
common location of /sys/firmware/security and new code would live in 
arch. This is my preference and would be the best fit for our use case.

2. Use securityfs.  This would mean modifying it to satisfy other use 
cases, including supporting userspace file creation. I don't know if the 
securityfs maintainer would find that acceptable. I would also still 
want some way to expose variables at /sys/firmware/security.

3. Use a sysfs-based approach. This would be a platform-specific 
implementation. However, sysfs has a similar issue to securityfs for 
file creation. When I tried it in RFC v1[1], I had to implement a 
workaround to achieve that.

[1] 
https://lore.kernel.org/linuxppc-dev/20220122005637.28199-3-nayna@linux.ibm.com/ (local)

Thanks & Regards,

      - Nayna