Re: [PATCH] iommu/arm-smmu: Fix out-of-bounds dereference

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

On Mon, Nov 07, 2016 at 03:39:02PM +0000, Robin Murphy wrote:
> When we iterate a master's config entries, what we generally care
> about is the entry's stream map index, rather than the entry index
> itself, so it's nice to have the iterator automatically assign the
> former from the latter. Unfortunately, booting with KASAN reveals
> the oversight that using a simple comma operator results in the
> entry index being dereferenced before being checked for validity,
> so we always access one element past the end of the fwspec array.
> 
> Flip things around so that the check always happens before the index
> may be dereferenced.
> 
> Fixes: adfec2e709d2 ("iommu/arm-smmu: Convert to iommu_fwspec")
> Reported-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f86683eec446..44ffe3a391d6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -324,8 +324,10 @@ struct arm_smmu_master_cfg {
>  #define INVALID_SMENDX			-1
>  #define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv)
>  #define fwspec_smmu(fw)  (__fwspec_cfg(fw)->smmu)
> -#define for_each_cfg_sme(fw, i, idx) \
> -	for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i)
> +#define for_each_cfg_sme(fw, i, idx)					\
> +	for (i = 0;							\
> +	     i < fw->num_ids && (idx = __fwspec_cfg(fw)->smendx[i], true); \
> +	     ++i)

Urgh, that's vile. Is it worth wrapping that in (yet another) macro, which
either returns the index or -ENOENT if i is out of bounds?

Will