Re: [OpenOCD-devel] [PATCH]: bab5952 Fix serious bug in LPC2xxx/LPC17xx flash algorithm.

[Andreas F. <and...@gm...>]

On Fri, Sep 28, 2012 at 10:46 PM, Freddie Chopin <fre...@op...> wrote:
> W dniu 2012-09-28 22:23, Andreas Fritiofson pisze:
>
>> That's exactly what I meant.
>>
>> In short, the user pointer that was removed was set up to the location
>> of the working area handle "returned" from the allocation function. On
>> free, it was set to NULL. In this case this location was the
>> lpc2000_info->iap_working_area field. So in effect, when the woring
>> area was freed on reset, the pointer would be NULL the next flashing
>> and the check would make sure it was reallocated and the algorithm
>> re-uploaded.
>>
>> Of course this could go horribly horribly wrong since the pointer used
>> to get the handle could just as well be a stack variable so it would
>> be out of scope when the free would set it to NULL.
>>
>> All flash code needs to be checked for this code construct. When I
>> wrote the allocator, the CFI code had lots of them, if I recall
>> correctly.
>
>
> So you suppose that there can be more of these? That's not very good /;
>
> Maybe removing of "user" field was not a good idea after all?
>
> 4\/3!!

I just checked and the same construct is used in several places. I
wouldn't want to reinstate the user pointer unless as a temporary fix,
though. The mechanism is flawed and dangerous. I've seen some uses
that could possibly corrupt stack with the user pointer thingy in
place (flash/nand/lpc...).

To find possible problem areas, search for instances of pointer to
struct working_area. Discard all that doesn't have static duration
(possibly as part of another struct). Check all uses of the remaining
ones for code that tries to reuse the area if the pointer is non-null.

I have a patch that has been collecting dust for months (years?) to
convert all working area pointers in flash structures to local
variables. Maybe it's time to polish it up. It probably fixes some of
the sites.

/Andreas