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

[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 -- geert@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

[PATCH 1/2] of: overlay: Fix out-of-bounds write in init_overlay_changeset()

[Geert Uytterhoeven]

If an overlay has no "__symbols__" node, but it has nodes without
"__overlay__" subnodes at the end (e.g. a "__fixups__" node), after
filling in all fragments for nodes with "__overlay__" subnodes,
"fragment = &fragments[cnt]" will point beyond the end of the allocated
array.

Hence writing to "fragment->overlay" will overwrite unallocated memory,
which may lead to a crash later.

Fix this by deferring both the assignment to "fragment" and the
offending write afterwards until we know for sure the node has an
"__overlay__" subnode, and thus a valid entry in "fragments[]".

Fixes: 61b4de4e0b384f4a ("of: overlay: minor restructuring")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
--
This applies to both v4.15-rc2 and Rob's for-next branch.
---
 drivers/of/overlay.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index b8918e92d5a5268f..49b8939af6b201ca 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -572,9 +572,10 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 	cnt = 0;
 	for_each_child_of_node(tree, node) {
-		fragment = &fragments[cnt];
-		fragment->overlay = of_get_child_by_name(node, "__overlay__");
-		if (fragment->overlay) {
+		overlay_node = of_get_child_by_name(node, "__overlay__");
+		if (overlay_node) {
+			fragment = &fragments[cnt];
+			fragment->overlay = overlay_node;
 			fragment->target = find_target_node(node);
 			if (!fragment->target) {
 				of_node_put(fragment->overlay);
-- 
2.7.4

[PATCH 2/2] of: overlay: Make node skipping in init_overlay_changeset() clearer

[Geert Uytterhoeven]

Make it more clear that nodes without "__overlay__" subnodes are
skipped, by reverting the logic and using continue.
This also reduces indentation level.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This applies to Rob's for-next branch only.
---
 drivers/of/overlay.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 49b8939af6b201ca..54a73bd771fe27b7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -573,18 +573,19 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	cnt = 0;
 	for_each_child_of_node(tree, node) {
 		overlay_node = of_get_child_by_name(node, "__overlay__");
-		if (overlay_node) {
-			fragment = &fragments[cnt];
-			fragment->overlay = overlay_node;
-			fragment->target = find_target_node(node);
-			if (!fragment->target) {
-				of_node_put(fragment->overlay);
-				ret = -EINVAL;
-				goto err_free_fragments;
-			}
+		if (!overlay_node)
+			continue;
 
-			cnt++;
+		fragment = &fragments[cnt];
+		fragment->overlay = overlay_node;
+		fragment->target = find_target_node(node);
+		if (!fragment->target) {
+			of_node_put(fragment->overlay);
+			ret = -EINVAL;
+			goto err_free_fragments;
 		}
+
+		cnt++;
 	}
 
 	/*
-- 
2.7.4

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

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 <robh@kernel.org> 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&id=efb72067c287cd6aba8eb434bd5bdc1ae0af6ed7
contains a few more lines than intended.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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 <frank.rowand@sony.com>


> 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 Frank,

On Sat, Dec 9, 2017 at 7:01 AM, Frank Rowand <frowand.list@gmail.com> 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 = <0xffffffff 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 <frank.rowand@sony.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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/09/17 01:04, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Sat, Dec 9, 2017 at 7:01 AM, Frank Rowand <frowand.list@gmail.com> 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 = <0xffffffff 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 <frank.rowand@sony.com>
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
>