[libcamera-devel] [PATCH 02/13] libcamera: ipa: Remove character restriction on the IPA name

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, May 03, 2023 at 01:20:24PM +0100, Naushir Patuck via libcamera-devel wrote:
> Remove the restriction on using the '/' character in the IPA name
> string. This allows more flexibility in IPA directory structures where
> different IPA modules might live in subdirectories under the usual
> src/ipa/<platform>/ top level directory.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/ipa_module.cpp | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index c152153c180f..3ca7074b3b58 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -225,9 +225,9 @@ Span<const uint8_t> elfLoadSymbol(Span<const uint8_t> elf, const char *symbol)
>   * \brief The name of the IPA module
>   *
>   * The name may be used to build file system paths to IPA-specific resources.
> - * It shall only contain printable characters, and may not contain '/', '*',
> - * '?' or '\'. For IPA modules included in libcamera, it shall match the
> - * directory of the IPA module in the source tree.
> + * It shall only contain printable characters, and may not contain '*', '?' or
> + * '\'. For IPA modules included in libcamera, it shall match the directory of
> + * the IPA module in the source tree.
>   *
>   * \todo Allow user to choose to isolate open source IPAs
>   */
> @@ -304,9 +304,8 @@ int IPAModule::loadIPAModuleInfo()
>  	std::string ipaName = info_.name;
>  	auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
>  				     [](unsigned char c) -> bool {
> -					     return isprint(c) && c != '/' &&
> -						    c != '?' && c != '*' &&
> -						    c != '\\';
> +					     return isprint(c) && c != '?' &&
> +						    c != '*' && c != '\\';

I think I mentioned in the review of the previous version that we should
disallow ".." to avoid escaping into the file system. I however have a
bit of a hard time at this point to decide if it would be best
implemented here or elsewhere, or what the exact naming rules should be.
I'll thus add a

 * \todo Consider module naming restrictions to avoid escaping from a base
 * directory. Forbidding ".." may be enough, but this may be best implemented
 * in a different layer.

comment.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  				     });
>  	if (iter != ipaName.end()) {
>  		LOG(IPAModule, Error)

-- 
Regards,

Laurent Pinchart