Re: [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 27/02/2021 00:26, Douglas Anderson wrote:
> When running the latest kernel on an sc7180 with KASAN I got this
> splat:
>    BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
>    Read of size 4 at addr ffffff8088f36100 by task kworker/7:1/58
>    CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
>    Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>    Workqueue: events deferred_probe_work_func
>    Call trace:
>     dump_backtrace+0x0/0x3a8
>     show_stack+0x24/0x30
>     dump_stack+0x174/0x1e0
>     print_address_description+0x70/0x2e4
>     kasan_report+0x178/0x1bc
>     __asan_report_load4_noabort+0x44/0x50
>     a6xx_gpu_init+0x618/0x644
>     adreno_bind+0x26c/0x438
> 
> This is because the speed bin is defined like this:
>    gpu_speed_bin: gpu_speed_bin@1d2 {
>      reg = <0x1d2 0x2>;
>      bits = <5 8>;
>    };
> 
> As you can see the "length" is 2 bytes. That means that the nvmem
> subsystem allocates only 2 bytes. The GPU code, however, was casting
> the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
> not so good.
> 
> Let's fix this to just use the nvmem_cell_read_u16() accessor function
> which simplifies things and also gets rid of the splat.
> 
> Let's also put an explicit conversion from little endian in place just
> to make things clear. The nvmem subsystem today is assuming little
> endian and this makes it clear. Specifically, the way the above sc7180
> cell is interpreted:
> 
> NVMEM:
>   +--------+--------+--------+--------+--------+
>   | ...... | 0x1d3  | 0x1d2  | ...... | 0x000  |
>   +--------+--------+--------+--------+--------+
>                ^       ^
>               msb     lsb
> 
> You can see that the least significant data is at the lower address
> which is little endian.
> 
> NOTE: someone who is truly paying attention might wonder about me
> picking the "u16" version of this accessor instead of the "u8" (since
> the value is 8 bits big) or the u32 version (just for fun). At the
> moment you need to pick the accessor that exactly matches the length
> the cell was specified as in the device tree. Hopefully future
> patches to the nvmem subsystem will fix this.
> 
> Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
>   1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index ba8e9d3cf0fe..0e2024defd79 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
>   		u32 revn)
>   {
>   	struct opp_table *opp_table;
> -	struct nvmem_cell *cell;
>   	u32 supp_hw = UINT_MAX;
> -	void *buf;
> -
> -	cell = nvmem_cell_get(dev, "speed_bin");
> -	/*
> -	 * -ENOENT means that the platform doesn't support speedbin which is
> -	 * fine
> -	 */
> -	if (PTR_ERR(cell) == -ENOENT)
> -		return 0;
> -	else if (IS_ERR(cell)) {
> -		DRM_DEV_ERROR(dev,
> -				"failed to read speed-bin. Some OPPs may not be supported by hardware");
> -		goto done;
> -	}
> +	u16 speedbin;
> +	int ret;
>   
> -	buf = nvmem_cell_read(cell, NULL);

I think the issue here is not passing len pointer which should return 
how many bytes the cell is!

Then from there we can decide to do le16_to_cpu or le32_to_cpu or not!
This will also future proof the code to handle speed_bins of different 
sizes!

--srini

> -	if (IS_ERR(buf)) {
> -		nvmem_cell_put(cell);

> +	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
> +	if (ret) {
>   		DRM_DEV_ERROR(dev,
> -				"failed to read speed-bin. Some OPPs may not be supported by hardware");
> +			      "failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
> +			      ret);
>   		goto done;
>   	}
> +	speedbin = le16_to_cpu(speedbin);
>   
> -	supp_hw = fuse_to_supp_hw(dev, revn, *((u32 *) buf));
> -
> -	kfree(buf);
> -	nvmem_cell_put(cell);
> +	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
>   
>   done:
>   	opp_table = dev_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>