[bug report] firmware: arm_ffa: Add schedule receiver callback mechanism

[bug report] firmware: arm_ffa: Add schedule receiver callback mechanism

[Dan Carpenter]

Hello Sudeep Holla,

The patch 0184450b8b1e: "firmware: arm_ffa: Add schedule receiver
callback mechanism" from Oct 5, 2023 (linux-next), leads to the
following Smatch static checker warning:

	drivers/firmware/arm_ffa/driver.c:1251 ffa_partitions_cleanup()
	warn: double check that we're allocating correct size: 8 vs 88

drivers/firmware/arm_ffa/driver.c
    1243 static void ffa_partitions_cleanup(void)
    1244 {
    1245         struct ffa_dev_part_info **info;
    1246         int idx, count = drv_info->partition_count;
    1247 
    1248         if (!count)
    1249                 return;
    1250 
--> 1251         info = kcalloc(count, sizeof(**info), GFP_KERNEL);

I *think* this should be sizeof(*info).  It ends up being a smaller
allocation (8 bytes instead of 88).

    1252         if (!info)
    1253                 return;
    1254 
    1255         xa_extract(&drv_info->partition_info, (void **)info, 0, VM_ID_MASK,

We copy count pointers to info.  We don't copy entire structs.  It still
works but it's larger than necessary.

    1256                    count, XA_PRESENT);
    1257 
    1258         for (idx = 0; idx < count; idx++)
    1259                 kfree(info[idx]);
    1260         kfree(info);
    1261 
    1262         drv_info->partition_count = 0;
    1263         xa_destroy(&drv_info->partition_info);
    1264 }

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] firmware: arm_ffa: Add schedule receiver callback mechanism

[Sudeep Holla]

On Mon, Oct 30, 2023 at 05:31:04PM +0300, Dan Carpenter wrote:
> Hello Sudeep Holla,
> 
> The patch 0184450b8b1e: "firmware: arm_ffa: Add schedule receiver
> callback mechanism" from Oct 5, 2023 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/firmware/arm_ffa/driver.c:1251 ffa_partitions_cleanup()
> 	warn: double check that we're allocating correct size: 8 vs 88
> 
> drivers/firmware/arm_ffa/driver.c
>     1243 static void ffa_partitions_cleanup(void)
>     1244 {
>     1245         struct ffa_dev_part_info **info;
>     1246         int idx, count = drv_info->partition_count;
>     1247 
>     1248         if (!count)
>     1249                 return;
>     1250 
> --> 1251         info = kcalloc(count, sizeof(**info), GFP_KERNEL);
> 
> I *think* this should be sizeof(*info).  It ends up being a smaller
> allocation (8 bytes instead of 88).

Not sure if I am following this warning properly. I am bit confused whether
it suggest 8 is correct or 88 is correct. Anyways, the expectation is to
just allocate 8 bytes for a pointer. We just fetch a list of stored pointer
in XArray and free them.

One possible way to avoid any confusion is to use sizeof(struct ffa_dev_part_info *)
or even sizeof(void *).

> 
>     1252         if (!info)
>     1253                 return;
>     1254 
>     1255         xa_extract(&drv_info->partition_info, (void **)info, 0, VM_ID_MASK,
> 
> We copy count pointers to info.  We don't copy entire structs.  It still
> works but it's larger than necessary.
>

Yes, that is the expected behaviour. We copy the pointers that were allocated
in the setup and free them here.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] firmware: arm_ffa: Add schedule receiver callback mechanism

[Dan Carpenter]

On Mon, Oct 30, 2023 at 04:01:07PM +0000, Sudeep Holla wrote:
> On Mon, Oct 30, 2023 at 05:31:04PM +0300, Dan Carpenter wrote:
> > Hello Sudeep Holla,
> > 
> > The patch 0184450b8b1e: "firmware: arm_ffa: Add schedule receiver
> > callback mechanism" from Oct 5, 2023 (linux-next), leads to the
> > following Smatch static checker warning:
> > 
> > 	drivers/firmware/arm_ffa/driver.c:1251 ffa_partitions_cleanup()
> > 	warn: double check that we're allocating correct size: 8 vs 88
> > 
> > drivers/firmware/arm_ffa/driver.c
> >     1243 static void ffa_partitions_cleanup(void)
> >     1244 {
> >     1245         struct ffa_dev_part_info **info;
> >     1246         int idx, count = drv_info->partition_count;
> >     1247 
> >     1248         if (!count)
> >     1249                 return;
> >     1250 
> > --> 1251         info = kcalloc(count, sizeof(**info), GFP_KERNEL);
> > 
> > I *think* this should be sizeof(*info).  It ends up being a smaller
> > allocation (8 bytes instead of 88).
> 
> Not sure if I am following this warning properly. I am bit confused whether
> it suggest 8 is correct or 88 is correct. Anyways, the expectation is to
> just allocate 8 bytes for a pointer. We just fetch a list of stored pointer
> in XArray and free them.
> 
> One possible way to avoid any confusion is to use sizeof(struct ffa_dev_part_info *)
> or even sizeof(void *).

The static checker is saying that 8 is correct but we are allocating 88
bytes.  There is an extra * in the sizeof().

I don't necessarily like to make buffers smaller in case I have
misunderstood the code, but it seems like we should do that here.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [bug report] firmware: arm_ffa: Add schedule receiver callback mechanism

[Sudeep Holla]

On Tue, Oct 31, 2023 at 07:15:45AM +0300, Dan Carpenter wrote:
> On Mon, Oct 30, 2023 at 04:01:07PM +0000, Sudeep Holla wrote:
> > On Mon, Oct 30, 2023 at 05:31:04PM +0300, Dan Carpenter wrote:
> > > Hello Sudeep Holla,
> > > 
> > > The patch 0184450b8b1e: "firmware: arm_ffa: Add schedule receiver
> > > callback mechanism" from Oct 5, 2023 (linux-next), leads to the
> > > following Smatch static checker warning:
> > > 
> > > 	drivers/firmware/arm_ffa/driver.c:1251 ffa_partitions_cleanup()
> > > 	warn: double check that we're allocating correct size: 8 vs 88
> > > 
> > > drivers/firmware/arm_ffa/driver.c
> > >     1243 static void ffa_partitions_cleanup(void)
> > >     1244 {
> > >     1245         struct ffa_dev_part_info **info;
> > >     1246         int idx, count = drv_info->partition_count;
> > >     1247 
> > >     1248         if (!count)
> > >     1249                 return;
> > >     1250 
> > > --> 1251         info = kcalloc(count, sizeof(**info), GFP_KERNEL);
> > > 
> > > I *think* this should be sizeof(*info).  It ends up being a smaller
> > > allocation (8 bytes instead of 88).
> > 
> > Not sure if I am following this warning properly. I am bit confused whether
> > it suggest 8 is correct or 88 is correct. Anyways, the expectation is to
> > just allocate 8 bytes for a pointer. We just fetch a list of stored pointer
> > in XArray and free them.
> > 
> > One possible way to avoid any confusion is to use sizeof(struct ffa_dev_part_info *)
> > or even sizeof(void *).
> 
> The static checker is saying that 8 is correct but we are allocating 88
> bytes.

OK 88 bytes was bit misleading for me initially but then realised that
when all the debug options are enables rwlock_t is 72bytes instead of 8bytes.
I was expecting 24 bytes in place 88 bytes.

> There is an extra * in the sizeof().
>

That said, I was completely blind about this. Sorry for that, clearly a
type that got missed so far.

> I don't necessarily like to make buffers smaller in case I have
> misunderstood the code, but it seems like we should do that here.
>

Agreed, sorry as I said I was confused with the report for other reason
as mentioned above as well as blindness to the typo I have made :D.

I will send a fix soon.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel