Re: [PATCH v5 09/11] FIT: fit_open: make filename handling more robust

[Marco Felsch <m.felsch@pengutronix.de>]

On 25-08-19, Sascha Hauer wrote:
> On Tue, Aug 19, 2025 at 10:19:41AM +0200, Marco Felsch wrote:
> > On 25-08-19, Sascha Hauer wrote:
> > > On Mon, Aug 18, 2025 at 07:26:17PM +0200, Marco Felsch wrote:
> > > > Require the filename to start at the root '/' directory and resolve any
> > > > possible link to make the filename handling more robust.
> > > > 
> > > > This is in preparation of adding cached fit_open support.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  common/image-fit.c | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/common/image-fit.c b/common/image-fit.c
> > > > index 0067f46e60bc954b418aef3398e2c10856b41c02..1cb407d4d86cb3d0a643149bb08c46caadcd56fe 100644
> > > > --- a/common/image-fit.c
> > > > +++ b/common/image-fit.c
> > > > @@ -972,12 +972,25 @@ struct fit_handle *fit_open_buf(const void *buf, size_t size, bool verbose,
> > > >   *
> > > >   * Return: A handle to a FIT image or a ERR_PTR
> > > >   */
> > > > -struct fit_handle *fit_open(const char *filename, bool verbose,
> > > > +struct fit_handle *fit_open(const char *_filename, bool verbose,
> > > >  			    enum bootm_verify verify, loff_t max_size)
> > > >  {
> > > >  	struct fit_handle *handle;
> > > > +	char *filename;
> > > >  	int ret;
> > > >  
> > > > +	if (*_filename != '/') {
> > > > +		pr_err("The FIT filename must start with '/'\n");
> > > > +		return ERR_PTR(-EINVAL);
> > > > +	}
> > > 
> > > Drop this. We want to be able to call fit_open() with relative pathes...
> > 
> > IMHO FIT containers are only used in verified-boot setups. I wouldn't
> > allow relative paths in such use-cases.
> 
> That's your/our usecase, but you don't know what others are doing and a
> "bootm foo.fit" should just work without complaining about a relative
> path.

Okay, albeit I'm still not convinced that 'bootm foo.fit' is a good
idea. But you're right, enforcing an absolute path may be a bit too
strict.

> > > > +
> > > > +	/* dirfd is ignored, since _filename is absolute */
> > > > +	filename = canonicalize_path(AT_FDCWD, _filename);
> > > 
> > > ...and you are calling canonicalize_path() anyway which gives you an
> > > absolute path from the potentially relative input path.
> > 
> > But we don't know the base directory to which this path would relative
> > too.
> 
> canonicalize_path() returns an absolute path.

I know and it does the job to resolve the whole path to use it for
caching purpose. But as said, I just using the CWD may trigger
unintentionally behaviors. Anyway, I will drop the '/' root restriction.

Regards,
  Marco

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>