linux-kernel - Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 21 Mar 2021 10:21:22 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Alex Elder <elder@...aro.org>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        bjorn.andersson@...aro.org, evgreen@...omium.org,
        cpratapa@...eaurora.org, subashab@...eaurora.org, elder@...nel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: ipa: fix IPA validation

On Sat, Mar 20, 2021 at 09:17:29AM -0500, Alex Elder wrote:
> There are blocks of IPA code that sanity-check various values, at
> compile time where possible.  Most of these checks can be done once
> during development but skipped for normal operation.  These checks
> permit the driver to make certain assumptions, thereby avoiding the
> need for runtime error checking.
> 
> The checks are defined conditionally, but not consistently.  In
> some cases IPA_VALIDATION enables the optional checks, while in
> others IPA_VALIDATE is used.
> 
> Fix this by using IPA_VALIDATION consistently.
> 
> Signed-off-by: Alex Elder <elder@...aro.org>
> ---
>  drivers/net/ipa/Makefile       | 2 +-
>  drivers/net/ipa/gsi_trans.c    | 8 ++++----
>  drivers/net/ipa/ipa_cmd.c      | 4 ++--
>  drivers/net/ipa/ipa_cmd.h      | 6 +++---
>  drivers/net/ipa/ipa_endpoint.c | 6 +++---
>  drivers/net/ipa/ipa_main.c     | 6 +++---
>  drivers/net/ipa/ipa_mem.c      | 6 +++---
>  drivers/net/ipa/ipa_table.c    | 6 +++---
>  drivers/net/ipa/ipa_table.h    | 6 +++---
>  9 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ipa/Makefile b/drivers/net/ipa/Makefile
> index afe5df1e6eeee..014ae36ac6004 100644
> --- a/drivers/net/ipa/Makefile
> +++ b/drivers/net/ipa/Makefile
> @@ -1,5 +1,5 @@
>  # Un-comment the next line if you want to validate configuration data
> -#ccflags-y		+=	-DIPA_VALIDATE
> +# ccflags-y		+=	-DIPA_VALIDATION

Maybe netdev folks think differently here, but general rule that dead
code and closed code is such, is not acceptable to in Linux kernel.

<...>

>  
> -#ifdef IPA_VALIDATE
> +#ifdef IPA_VALIDATION
>  	if (!size || size % 8)
>  		return -EINVAL;
>  	if (count < max_alloc)
>  		return -EINVAL;
>  	if (!max_alloc)
>  		return -EINVAL;
> -#endif /* IPA_VALIDATE */
> +#endif /* IPA_VALIDATION */

If it is possible to supply those values, the check should be always and
not only under some closed config option.

>  
>  	/* By allocating a few extra entries in our pool (one less
>  	 * than the maximum number that will be requested in a
> @@ -140,14 +140,14 @@ int gsi_trans_pool_init_dma(struct device *dev, struct gsi_trans_pool *pool,
>  	dma_addr_t addr;
>  	void *virt;
>  
> -#ifdef IPA_VALIDATE
> +#ifdef IPA_VALIDATION
>  	if (!size || size % 8)
>  		return -EINVAL;
>  	if (count < max_alloc)
>  		return -EINVAL;
>  	if (!max_alloc)
>  		return -EINVAL;
> -#endif /* IPA_VALIDATE */
> +#endif /* IPA_VALIDATION */

Same

<...>

>  {
> -#ifdef IPA_VALIDATE
> +#ifdef IPA_VALIDATION
>  	/* At one time we assumed a 64-bit build, allowing some do_div()
>  	 * calls to be replaced by simple division or modulo operations.
>  	 * We currently only perform divide and modulo operations on u32,
> @@ -768,7 +768,7 @@ static void ipa_validate_build(void)
>  	BUILD_BUG_ON(!ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY));
>  	BUILD_BUG_ON(ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY) >
>  			field_max(AGGR_GRANULARITY_FMASK));
> -#endif /* IPA_VALIDATE */
> +#endif /* IPA_VALIDATION */

BUILD_BUG_ON()s are checked during compilation and not during runtime
like IPA_VALIDATION promised.

IMHO, the issue here is that this IPA code isn't release quality but
some debug drop variant and it is far from expected from submitted code.

Thanks