Re: [PATCH 3/3] startup: mount ps only on policy FS_EXTERNAL

[Fabian Pflug <f.pflug@pengutronix.de>]

Hey :)

On Tue, 2025-10-28 at 13:56 +0100, Ahmad Fatoum wrote:
> Hi,
> 
> On 10/28/25 1:18 PM, Fabian Pflug wrote:
> > Without the SCONFIG_FS_EXTERNAL, the bus of the driver for pstore will
> > not load, resulting in a missing driver for pstore and an error during
> > bootup.
> > Only mount the /pstore if FS_EXTERNAL is allowed by the security policy.
> > > Signed-off-by: Fabian Pflug <f.pflug@pengutronix.de>
> > ---
> >  common/startup.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/startup.c b/common/startup.c
> > index ea5436afa6..f16a99f7e4 100644
> > --- a/common/startup.c
> > +++ b/common/startup.c
> > @@ -55,6 +55,19 @@ extern exitcall_t __barebox_exitcalls_start[], __barebox_exitcalls_end[];
> >  enum system_states barebox_system_state;
> >  
> >  #if defined CONFIG_FS_RAMFS && defined CONFIG_FS_DEVFS
> > +static struct sconfig_notifier_block sconfig_notifier;
> > +static void u_mount_pstore(struct sconfig_notifier_block *nb,
> > +			   enum security_config_option opt, bool allowed)
> > +{
> > +	if (allowed) {
> > +		mkdir("/pstore", 0);
> 
> I think we should create the directory unconditionally without paying
> respect to whether we can mount or not.

I think that could irritate people into thinking, that there is nothing inside the pstore.
When there is no /pstore directory, it is clear, that the mount is not there and one gets to wonder why and what happens
here instead of chasing around, why the pstore is empty.

> 
> > +		mount("none", "pstore", "/pstore", NULL);
> > +	} else {
> > +		umount("/pstore");
> 
> The harm is already done when loosening security mode, so I don't think
> we want to start unmounting things.

That I can do. :)

> 
> As mentioned, I'd prefer replacing SCONFIG_FS_EXTERNAL altogether with a
> whitelist of mounts, so common code calls:
> 
>   allow_mount("none", "ramfs", "/", NULL);
>   allow_mount("none", "devfs", "/dev", NULL);
> 
> and board code can call:
> 
>   allow_mount("non", "pstore", "/pstore", NULL);
> 
> and we won't need any special handling here. Waiting to see what Sascha
> thinks.


I see your Idea and think this could be good, but also need something fast for me now and this works in that regard. I
see yours as a long term solution that is a bit more complicated that what I know about barebox.

Kind regards
Fabian

> 
> Cheers,
> Ahmad
> 
> > +		rmdir("/pstore");
> > +	}
> > +}
> > +
> >  static int mount_root(void)
> >  {
> >  	mount("none", "ramfs", "/", NULL);
> > @@ -69,8 +82,13 @@ static int mount_root(void)
> >  	}
> >  
> >  	if (IS_ENABLED(CONFIG_FS_PSTORE)) {
> > -		mkdir("/pstore", 0);
> > -		mount("none", "pstore", "/pstore", NULL);
> > +		if (IS_ALLOWED(SCONFIG_FS_EXTERNAL)) {
> > +			mkdir("/pstore", 0);
> > +			mount("none", "pstore", "/pstore", NULL);
> > +		}
> > +		sconfig_register_handler_filtered(&sconfig_notifier,
> > +						  u_mount_pstore,
> > +						  SCONFIG_FS_EXTERNAL);
> >  	}
> >  
> >  	if (IS_ENABLED(CONFIG_9P_FS))