Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Frank Rowand]

On 12/09/17 01:04, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Sat, Dec 9, 2017 at 7:01 AM, Frank Rowand  wrote:
>> On 12/08/17 05:13, Geert Uytterhoeven wrote:
>>> This patch series fixes memory corruption when applying overlays.
>>> I first noticed this when using OF configfs.  After lots of failed
>>> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
>>> attributes", which is not upstream.  But that was a red herring: that
>>> commit enlarged struct fragment to exactly 64-bytes, which just made it
>>> more likely to cause random corruption when writing beyond the end of an
>>> array of fragment structures.  With the smaller structure size before,
>>> such writes usually ended up in the unused holes between allocated
>>> blocks, causing no harm.
>>>
>>> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
>>> for-next branch.
>>> The second patch is a small improvement, and applies to Rob's for-next
>>> branch only.
>>
>> Overlay FDT files should not have invalid contents.  But they inevitably
>> will, so the code has to handle those cases.  Thanks for finding this
>> problem and making the code better!
> 
> Sure, people can throw anything at it ;-)
> 
> In my case, I'm wondering if the dtbo was actually invalid?
> Simplification of the decompiled dtbo:
> 
> /dts-v1/;
> 
> / {
> 
> fragment-name {
> target-path = [2f 00];
> 
> __overlay__ {
> 
> node-name {
> compatible = "foo,bar";
> gpios = <0x 0x0 0x0>;
> };
> };
> };
> 
> __fixups__ {
> bank0 = "/fragment-name/__overlay__/node-name:gpios:0";
> };
> };
> 
> So it has __fixup__, but no __symbols__, which looks totally valid to me.

Yes, that is correct.  The bug would also be exposed if there was a 
__local_fixups__
node without a __symbols__ node.  Which is also a valid overlay.

My comment was triggered by another possible case, where a non-overlay node
occurs in an overlay, without a __symbols__ node.  I'm not positive, but I
don't think that dtc would find an error in that case. 


>> For the full series:
>>
>> Reviewed-by: Frank Rowand 
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Geert Uytterhoeven]

Hi Frank,

On Sat, Dec 9, 2017 at 7:01 AM, Frank Rowand  wrote:
> On 12/08/17 05:13, Geert Uytterhoeven wrote:
>> This patch series fixes memory corruption when applying overlays.
>> I first noticed this when using OF configfs.  After lots of failed
>> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
>> attributes", which is not upstream.  But that was a red herring: that
>> commit enlarged struct fragment to exactly 64-bytes, which just made it
>> more likely to cause random corruption when writing beyond the end of an
>> array of fragment structures.  With the smaller structure size before,
>> such writes usually ended up in the unused holes between allocated
>> blocks, causing no harm.
>>
>> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
>> for-next branch.
>> The second patch is a small improvement, and applies to Rob's for-next
>> branch only.
>
> Overlay FDT files should not have invalid contents.  But they inevitably
> will, so the code has to handle those cases.  Thanks for finding this
> problem and making the code better!

Sure, people can throw anything at it ;-)

In my case, I'm wondering if the dtbo was actually invalid?
Simplification of the decompiled dtbo:

/dts-v1/;

/ {

fragment-name {
target-path = [2f 00];

__overlay__ {

node-name {
compatible = "foo,bar";
gpios = <0x 0x0 0x0>;
};
};
};

__fixups__ {
bank0 = "/fragment-name/__overlay__/node-name:gpios:0";
};
};

So it has __fixup__, but no __symbols__, which looks totally valid to me.

> For the full series:
>
> Reviewed-by: Frank Rowand 

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Frank Rowand]

On 12/08/17 05:13, Geert Uytterhoeven wrote:
>   Hi Pantelis, Rob, Frank,
> 
> This patch series fixes memory corruption when applying overlays.
> 
> I first noticed this when using OF configfs.  After lots of failed
> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
> attributes", which is not upstream.  But that was a red herring: that
> commit enlarged struct fragment to exactly 64-bytes, which just made it
> more likely to cause random corruption when writing beyond the end of an
> array of fragment structures.  With the smaller structure size before,
> such writes usually ended up in the unused holes between allocated
> blocks, causing no harm.
> 
> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
> for-next branch.
> The second patch is a small improvement, and applies to Rob's for-next
> branch only.

Overlay FDT files should not have invalid contents.  But they inevitably
will, so the code has to handle those cases.  Thanks for finding this
problem and making the code better!

For the full series:

Reviewed-by: Frank Rowand 


> I've updated my topic/overlays and topic/renesas-overlays branches at
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
> accordingly.
> 
> Thanks!
> 
> Geert Uytterhoeven (2):
>   of: overlay: Fix out-of-bounds write in init_overlay_changeset()
>   of: overlay: Make node skipping in init_overlay_changeset() clearer
> 
>  drivers/of/overlay.c | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Geert Uytterhoeven]

Hi Rob,

On Fri, Dec 8, 2017 at 4:11 PM, Rob Herring  wrote:
> On Fri, Dec 08, 2017 at 02:13:01PM +0100, Geert Uytterhoeven wrote:
>> This patch series fixes memory corruption when applying overlays.
>>
>> I first noticed this when using OF configfs.  After lots of failed
>> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
>> attributes", which is not upstream.  But that was a red herring: that
>> commit enlarged struct fragment to exactly 64-bytes, which just made it
>> more likely to cause random corruption when writing beyond the end of an
>> array of fragment structures.  With the smaller structure size before,
>> such writes usually ended up in the unused holes between allocated
>> blocks, causing no harm.
>>
>> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
>> for-next branch.
>> The second patch is a small improvement, and applies to Rob's for-next
>> branch only.
>>
>> I've updated my topic/overlays and topic/renesas-overlays branches at
>> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
>> accordingly.
>>
>> Thanks!
>>
>> Geert Uytterhoeven (2):
>>   of: overlay: Fix out-of-bounds write in init_overlay_changeset()
>>   of: overlay: Make node skipping in init_overlay_changeset() clearer
>
> I've applied both and am updating my pull req to Linus. I hope that's
> the end of it. If further fixes can't be reproduced with mainline, I'm
> not going to be inclined to take them for 4.15.

Tahnks!

BTW, seems I accidentally used "--" instead of "---" as a separator, so the
commit message
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/linus=efb72067c287cd6aba8eb434bd5bdc1ae0af6ed7
contains a few more lines than intended.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 0/2] of: overlay: Crash fix and improvement

[Rob Herring]

On Fri, Dec 08, 2017 at 02:13:01PM +0100, Geert Uytterhoeven wrote:
>   Hi Pantelis, Rob, Frank,
> 
> This patch series fixes memory corruption when applying overlays.
> 
> I first noticed this when using OF configfs.  After lots of failed
> debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
> attributes", which is not upstream.  But that was a red herring: that
> commit enlarged struct fragment to exactly 64-bytes, which just made it
> more likely to cause random corruption when writing beyond the end of an
> array of fragment structures.  With the smaller structure size before,
> such writes usually ended up in the unused holes between allocated
> blocks, causing no harm.
> 
> The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
> for-next branch.
> The second patch is a small improvement, and applies to Rob's for-next
> branch only.
> 
> I've updated my topic/overlays and topic/renesas-overlays branches at
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
> accordingly.
> 
> Thanks!
> 
> Geert Uytterhoeven (2):
>   of: overlay: Fix out-of-bounds write in init_overlay_changeset()
>   of: overlay: Make node skipping in init_overlay_changeset() clearer

I've applied both and am updating my pull req to Linus. I hope that's 
the end of it. If further fixes can't be reproduced with mainline, I'm 
not going to be inclined to take them for 4.15.

Rob

[PATCH 0/2] of: overlay: Crash fix and improvement

[Geert Uytterhoeven]

Hi Pantelis, Rob, Frank,

This patch series fixes memory corruption when applying overlays.

I first noticed this when using OF configfs.  After lots of failed
debugging attempts, I bisected it to "of: overlay: add per overlay sysfs
attributes", which is not upstream.  But that was a red herring: that
commit enlarged struct fragment to exactly 64-bytes, which just made it
more likely to cause random corruption when writing beyond the end of an
array of fragment structures.  With the smaller structure size before,
such writes usually ended up in the unused holes between allocated
blocks, causing no harm.

The first patch is the real fix, and applies to both v4.15-rc2 and Rob's
for-next branch.
The second patch is a small improvement, and applies to Rob's for-next
branch only.

I've updated my topic/overlays and topic/renesas-overlays branches at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git
accordingly.

Thanks!

Geert Uytterhoeven (2):
  of: overlay: Fix out-of-bounds write in init_overlay_changeset()
  of: overlay: Make node skipping in init_overlay_changeset() clearer

 drivers/of/overlay.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

-- 
2.7.4

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds