linux-kernel - Re: [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink

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:   Wed, 15 Nov 2023 16:43:49 +0000
From:   James Clark <james.clark@....com>
To:     Junhao He <hejunhao3@...wei.com>, suzuki.poulose@....com,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        jonathan.cameron@...wei.com, yangyicong@...wei.com,
        prime.zeng@...ilicon.com
Subject: Re: [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before
 register sink



On 14/11/2023 13:33, Junhao He wrote:
> The SMB dirver register the enable/disable sysfs interface in function
> smb_register_sink(), however the buffer depends on the following
> configuration to work well. So it'll be possible for user to access an
> unreset one.
> 
> Move the config buffer operation to before register_sink().
> Ignore the return value, if smb_config_inport() fails. That will
> cause the hardwares disable trace path to fail, should not affect
> SMB driver remove. So we make smb_remove() return success,
> 
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@...wei.com>
> ---
>  drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 0a0fe9fcc57f..2f2aba90a514 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = smb_config_inport(dev, true);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, drvdata);
>  	spin_lock_init(&drvdata->spinlock);
>  	drvdata->pid = -1;
>  
>  	ret = smb_register_sink(pdev, drvdata);
>  	if (ret) {
> +		smb_config_inport(&pdev->dev, false);
>  		dev_err(dev, "Failed to register SMB sink\n");
>  		return ret;
>  	}
>  
> -	ret = smb_config_inport(dev, true);
> -	if (ret) {
> -		smb_unregister_sink(drvdata);
> -		return ret;
> -	}
> -
> -	platform_set_drvdata(pdev, drvdata);
> -
>  	return 0;
>  }
>  
>  static int smb_remove(struct platform_device *pdev)
>  {
>  	struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
> -	int ret;
> -
> -	ret = smb_config_inport(&pdev->dev, false);
> -	if (ret)
> -		return ret;
>  
>  	smb_unregister_sink(drvdata);
>  
> +	smb_config_inport(&pdev->dev, false);
> +
>  	return 0;
>  }
>  

I'm not sure if it makes sense to implement Uwe's change to use
.remove_new instead here? And then drop the change on the other
patchset. Otherwise Suzuki will have to resolve the conflict. Maybe it's
quite simple so it's not an issue.

Either way:

Reviewed-by: James Clark <james.clark@....com>