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

["Hegde, Suma" <Suma.Hegde@amd.com>]

Hi Hans,

On 2/19/2024 6:18 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
Thank you for your review comment, I will send a patch to address this 
change.
>> @@ -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"

Ilpo has already merged it into relevant patch(platform/x86/amd/hsmp: 
Non-ACPI support for AMD F1A_M00~0Fh) and the change is available in 
review-ilpo branch.

> Regards,
>
> Hans


Thanks and Regards,

Suma

>