[PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Dan Carpenter]

The "sz" variable needs to be a signed type for the error handling to
work as intended.  Fortunately, there is some sanity checking on "sz" on
the next line, so negative values would be caught and it doesn't really
affect runtime.

Fixes: eab0dce11dd9 ("scsi: ufs: ufshcd-pltfrm: Use of_property_count_u32_elems() to get property length")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/ufs/host/ufshcd-pltfrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index 0c9b303ccfa0..1f4f30d6cb42 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -31,7 +31,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
 	const char *name;
 	u32 *clkfreq = NULL;
 	struct ufs_clk_info *clki;
-	size_t sz = 0;
+	ssize_t sz = 0;
 
 	if (!np)
 		goto out;
-- 
2.43.0

Re: [PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Bart Van Assche]

On 8/15/24 4:24 AM, Dan Carpenter wrote:
> The "sz" variable needs to be a signed type for the error handling to
> work as intended.

What error handling are you referring to? I haven't found any code that
assigns a negative value to 'sz' in ufshcd_parse_clock_info(). Did I
perhaps overlook something?

Thanks,

Bart.

Re: [PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Dan Carpenter]

On Thu, Aug 15, 2024 at 10:47:30AM -0700, Bart Van Assche wrote:
> On 8/15/24 4:24 AM, Dan Carpenter wrote:
> > The "sz" variable needs to be a signed type for the error handling to
> > work as intended.
> 
> What error handling are you referring to? I haven't found any code that
> assigns a negative value to 'sz' in ufshcd_parse_clock_info(). Did I
> perhaps overlook something?
> 

Rob's patch in linux-next.

-       if (!of_get_property(np, "freq-table-hz", &len)) {
+       sz = of_property_count_u32_elems(np, "freq-table-hz");
+       if (sz <= 0) {
                dev_info(dev, "freq-table-hz property not specified\n");
                goto out;

regards,
dan carpenter

Re: [PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Manivannan Sadhasivam]

On Fri, Aug 16, 2024 at 12:35:22AM +0300, Dan Carpenter wrote:
> On Thu, Aug 15, 2024 at 10:47:30AM -0700, Bart Van Assche wrote:
> > On 8/15/24 4:24 AM, Dan Carpenter wrote:
> > > The "sz" variable needs to be a signed type for the error handling to
> > > work as intended.
> > 
> > What error handling are you referring to? I haven't found any code that
> > assigns a negative value to 'sz' in ufshcd_parse_clock_info(). Did I
> > perhaps overlook something?
> > 
> 
> Rob's patch in linux-next.
> 

It would've been helpful if you added 'next' in the patch subject prefix.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> -       if (!of_get_property(np, "freq-table-hz", &len)) {
> +       sz = of_property_count_u32_elems(np, "freq-table-hz");
> +       if (sz <= 0) {
>                 dev_info(dev, "freq-table-hz property not specified\n");
>                 goto out;
> 
> regards,
> dan carpenter
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Dan Carpenter]

On Fri, Aug 16, 2024 at 12:04:04PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 16, 2024 at 12:35:22AM +0300, Dan Carpenter wrote:
> > On Thu, Aug 15, 2024 at 10:47:30AM -0700, Bart Van Assche wrote:
> > > On 8/15/24 4:24 AM, Dan Carpenter wrote:
> > > > The "sz" variable needs to be a signed type for the error handling to
> > > > work as intended.
> > > 
> > > What error handling are you referring to? I haven't found any code that
> > > assigns a negative value to 'sz' in ufshcd_parse_clock_info(). Did I
> > > perhaps overlook something?
> > > 
> > 
> > Rob's patch in linux-next.
> > 
> 
> It would've been helpful if you added 'next' in the patch subject prefix.
> 

I guess that would helped in this case.  But most of the time when I see this
question it's because there are two different upstream maintainers modifying the
same code...  Anyway, sure, I can change my script to add "next" to the subject
when the FIXES_COMMIT isn't in Linus's tree.

if [ "$FIXES_COMMIT" != "" ] ; then
    if ! git merge-base --is-ancestor $FIXES_COMMIT origin/master ; then
        TREE=" next"
    fi
fi

regards,
dan carpenter

Re: [PATCH] scsi: ufs: ufshcd-pltfrm: Signedness bug in ufshcd_parse_clock_info()

[Martin K. Petersen]

Dan,

> The "sz" variable needs to be a signed type for the error handling to
> work as intended. Fortunately, there is some sanity checking on "sz"
> on the next line, so negative values would be caught and it doesn't
> really affect runtime.

Applied to 6.12/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering