| Message ID | 20240130073415.3391685-2-suma.hegde@amd.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/x86/amd/hsmp: Remove NULL dereferencing code | expand |
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
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 >
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; @@ -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;