Re: [PATCH v2] platform/x86/amd/hsmp: Remove NULL dereferencing code

[Hans de Goede <hdegoede@redhat.com>]

Hi Suma,

On 1/30/24 08:34, Suma Hegde wrote:
> Do not log using dev_err() in case of !sock, which causes null pointer
> dereferencing.
> 
> Also remove unnecessary check "boot_cpu_data.x86_model >= 0x00", which is
> always true because its an unsigned type.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202401292056.qkUFS09Y-lkp@intel.com/
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202401291311.gzMCj6SP-lkp@intel.com/
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v1:
> Correct the email id for Naveen Krishna Chatradhi.
> 
>  drivers/platform/x86/amd/hsmp.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 1baddf403920..1927be901108 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -566,17 +566,15 @@ static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
>  	struct hsmp_message msg = { 0 };
>  	int ret;
>  
> +	if (!sock)
> +		return -EINVAL;
> +
>  	/* Do not support lseek(), reads entire metric table */
>  	if (count < bin_attr->size) {
>  		dev_err(sock->dev, "Wrong buffer size\n");
>  		return -EINVAL;
>  	}
>  
> -	if (!sock) {
> -		dev_err(sock->dev, "Failed to read attribute private data\n");
> -		return -EINVAL;
> -	}
> -
>  	msg.msg_id	= HSMP_GET_METRIC_TABLE;
>  	msg.sock_ind	= sock->sock_ind;
> 

sock gets initialized like this:

        struct hsmp_socket *sock = bin_attr->private;

and bin_attr->private is setup before registering the file
and thus it will never be NULL.

So the correct fix would be to simply drop the check
rather then to move it.

> @@ -739,8 +737,7 @@ static int hsmp_cache_proto_ver(u16 sock_ind)
>  
>  static inline bool is_f1a_m0h(void)
>  {
> -	if (boot_cpu_data.x86 == 0x1A &&
> -	    (boot_cpu_data.x86_model >= 0x00 && boot_cpu_data.x86_model <= 0x0F))
> +	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
>  		return true;
>  
>  	return false;

This bit looks fine but this really belongs in a separate patch
as it has nothing to do with "Remove NULL dereferencing code"

Regards,

Hans