[BUG] double free in dfu_free_entities()

[BUG] double free in dfu_free_entities()

[Heinrich Schuchardt]

Hello Lukasz,

Jose and I have observed segmentation violations when
dfu_free_entities() is called.

In our scenario we have:

dfu_alt_info=
sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000

dfu_free_entities() is called with these entries in dfu:

u-boot-env:
  dfu->data.sf.dev 0000000015baf420 m25p16
  dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0
u-boot-bin:
  dfu->data.sf.dev 0000000015baf420 m25p16
  dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0

The same device is released twice which eventually leads to the
segmentation fault in device_chld_remove().

Program received signal SIGSEGV, Segmentation fault.
device_chld_remove (
dev=dev at entry=0x5555557f6750 <av_+1936>, drv=drv at entry=0x0,
flags=flags at entry=1)
at drivers/core/device-remove.c:55
55 list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {

We must ensure that dfu_free_entities() removes each device only once.

Another bug in dfu_free_entities() is that only the first list member is
freed. free() should be called for all list members.

Best regards

Heinrich

[BUG] double free in dfu_free_entities()

[Lukasz Majewski]

Hi Heinrich,

> Hello Lukasz,
> 
> Jose and I have observed segmentation violations when
> dfu_free_entities() is called.
> 

Thanks for finding them.

> In our scenario we have:
> 
> dfu_alt_info=
> sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000
> 0x200000
> 
> dfu_free_entities() is called with these entries in dfu:
> 
> u-boot-env:
>   dfu->data.sf.dev 0000000015baf420 m25p16
>   dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0
> u-boot-bin:
>   dfu->data.sf.dev 0000000015baf420 m25p16
>   dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0
> 
> The same device is released twice which eventually leads to the
> segmentation fault in device_chld_remove().

If I remember correctly that was fixed some time ago... Maybe something
has changed with recent patches.

Was it working before? Or is the above scenario a new one?

> 
> Program received signal SIGSEGV, Segmentation fault.
> device_chld_remove (
> dev=dev at entry=0x5555557f6750 <av_+1936>, drv=drv at entry=0x0,
> flags=flags at entry=1)
> at drivers/core/device-remove.c:55
> 55 list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
> 
> We must ensure that dfu_free_entities() removes each device only once.
> 
> Another bug in dfu_free_entities() is that only the first list member
> is freed. free() should be called for all list members.

Yes. Correct. 

> 
> Best regards
> 
> Heinrich
> 
> 
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210311/327d7310/attachment.sig>

[BUG] double free in dfu_free_entities()

[Heinrich Schuchardt]

On 3/11/21 9:18 AM, Lukasz Majewski wrote:
> Hi Heinrich,
>
>> Hello Lukasz,
>>
>> Jose and I have observed segmentation violations when
>> dfu_free_entities() is called.
>>
>
> Thanks for finding them.
>
>> In our scenario we have:
>>
>> dfu_alt_info=
>> sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000
>> 0x200000
>>
>> dfu_free_entities() is called with these entries in dfu:
>>
>> u-boot-env:
>>    dfu->data.sf.dev 0000000015baf420 m25p16
>>    dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0
>> u-boot-bin:
>>    dfu->data.sf.dev 0000000015baf420 m25p16
>>    dfu->data.sf.dev->dev 0000000015888410 spi.bin at 0
>>
>> The same device is released twice which eventually leads to the
>> segmentation fault in device_chld_remove().
>
> If I remember correctly that was fixed some time ago... Maybe something
> has changed with recent patches.
>
> Was it working before? Or is the above scenario a new one?

Jose and Takahiro have provided patches that exercise DFU by repeatedly
calling dfu_init_env_entities() and dfu_free_entities(). See function
efi_get_dfu_info().

Furthermore for testing we use a dfu_alt_info with multiple images on
the same device.

No change was done in this part of DFU since 2014. So this is a
longstanding problem that we now uncovered.

[PATCH 1/1] mtd: spi_flash_free()
https://lists.denx.de/pipermail/u-boot/2021-March/443915.html

fixes the issue for the case of DM_SPI_FLASH.

But we should also address the DFU system itself.

Best regards

Heinrich

>
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> device_chld_remove (
>> dev=dev at entry=0x5555557f6750 <av_+1936>, drv=drv at entry=0x0,
>> flags=flags at entry=1)
>> at drivers/core/device-remove.c:55
>> 55 list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
>>
>> We must ensure that dfu_free_entities() removes each device only once.
>>
>> Another bug in dfu_free_entities() is that only the first list member
>> is freed. free() should be called for all list members.
>
> Yes. Correct.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>
>>
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
>