From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751296AbdBWT44 (ORCPT ); Thu, 23 Feb 2017 14:56:56 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:35605 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751213AbdBWT4y (ORCPT ); Thu, 23 Feb 2017 14:56:54 -0500 Subject: Re: [RFC/PATCH] of: Mark property::value as const To: Stephen Boyd , Rob Herring References: <20170214025040.23955-1-stephen.boyd@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org From: Frank Rowand Message-ID: <58AF3E06.4030701@gmail.com> Date: Thu, 23 Feb 2017 11:54:46 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20170214025040.23955-1-stephen.boyd@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/13/17 18:50, Stephen Boyd wrote: > The 'blob' we pass into populate_properties() is marked as const, > but we cast that const away when we assign the result of > fdt_getprop_by_offset() to pp->value. Let's mark value as const > instead, so that code can't mistakenly write to the value of the > property that we've so far advertised as const. > > Unfortunately, this exposes a problem with the fdt resolver code, > where we overwrite the value member of properties of phandles to > update them with their final value. Add a comment for now to > indicate where we're potentially writing over const data. The resolver should not be over writing anything in the FDT. I'll look at what is going on there. The FDT we expose to user space should be the FDT we booted with, not something later modified. -Frank > > You can see the problem here by loading an overlay dtb into > the kernel via the request firmware helper method (not direct > loading) and then passing that tree to the resolver on an arm64 > device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO > and the code crashes when attempting to write to the blob to update > the phandle properties. > > Signed-off-by: Stephen Boyd > --- > > I was thinking perhaps it would work to store another __be32 variant > of the phandle in each device node, but then we still have a problem > with properties that have phandles inside them at some offset that we > need to update. I guess the only real solution is to deep copy the > property in that case and then save around some info to free the > duplicated property later on? > > drivers/of/base.c | 2 +- > drivers/of/fdt.c | 12 ++++++------ > drivers/of/resolver.c | 3 +++ > include/linux/of.h | 2 +- > 4 files changed, 11 insertions(+), 8 deletions(-) < snip > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Frank Rowand Subject: Re: [RFC/PATCH] of: Mark property::value as const Date: Thu, 23 Feb 2017 11:54:46 -0800 Message-ID: <58AF3E06.4030701@gmail.com> References: <20170214025040.23955-1-stephen.boyd@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170214025040.23955-1-stephen.boyd@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Stephen Boyd , Rob Herring Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org On 02/13/17 18:50, Stephen Boyd wrote: > The 'blob' we pass into populate_properties() is marked as const, > but we cast that const away when we assign the result of > fdt_getprop_by_offset() to pp->value. Let's mark value as const > instead, so that code can't mistakenly write to the value of the > property that we've so far advertised as const. > > Unfortunately, this exposes a problem with the fdt resolver code, > where we overwrite the value member of properties of phandles to > update them with their final value. Add a comment for now to > indicate where we're potentially writing over const data. The resolver should not be over writing anything in the FDT. I'll look at what is going on there. The FDT we expose to user space should be the FDT we booted with, not something later modified. -Frank > > You can see the problem here by loading an overlay dtb into > the kernel via the request firmware helper method (not direct > loading) and then passing that tree to the resolver on an arm64 > device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO > and the code crashes when attempting to write to the blob to update > the phandle properties. > > Signed-off-by: Stephen Boyd > --- > > I was thinking perhaps it would work to store another __be32 variant > of the phandle in each device node, but then we still have a problem > with properties that have phandles inside them at some offset that we > need to update. I guess the only real solution is to deep copy the > property in that case and then save around some info to free the > duplicated property later on? > > drivers/of/base.c | 2 +- > drivers/of/fdt.c | 12 ++++++------ > drivers/of/resolver.c | 3 +++ > include/linux/of.h | 2 +- > 4 files changed, 11 insertions(+), 8 deletions(-) < snip > From mboxrd@z Thu Jan 1 00:00:00 1970 From: frowand.list@gmail.com (Frank Rowand) Date: Thu, 23 Feb 2017 11:54:46 -0800 Subject: [RFC/PATCH] of: Mark property::value as const In-Reply-To: <20170214025040.23955-1-stephen.boyd@linaro.org> References: <20170214025040.23955-1-stephen.boyd@linaro.org> Message-ID: <58AF3E06.4030701@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 02/13/17 18:50, Stephen Boyd wrote: > The 'blob' we pass into populate_properties() is marked as const, > but we cast that const away when we assign the result of > fdt_getprop_by_offset() to pp->value. Let's mark value as const > instead, so that code can't mistakenly write to the value of the > property that we've so far advertised as const. > > Unfortunately, this exposes a problem with the fdt resolver code, > where we overwrite the value member of properties of phandles to > update them with their final value. Add a comment for now to > indicate where we're potentially writing over const data. The resolver should not be over writing anything in the FDT. I'll look at what is going on there. The FDT we expose to user space should be the FDT we booted with, not something later modified. -Frank > > You can see the problem here by loading an overlay dtb into > the kernel via the request firmware helper method (not direct > loading) and then passing that tree to the resolver on an arm64 > device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO > and the code crashes when attempting to write to the blob to update > the phandle properties. > > Signed-off-by: Stephen Boyd > --- > > I was thinking perhaps it would work to store another __be32 variant > of the phandle in each device node, but then we still have a problem > with properties that have phandles inside them at some offset that we > need to update. I guess the only real solution is to deep copy the > property in that case and then save around some info to free the > duplicated property later on? > > drivers/of/base.c | 2 +- > drivers/of/fdt.c | 12 ++++++------ > drivers/of/resolver.c | 3 +++ > include/linux/of.h | 2 +- > 4 files changed, 11 insertions(+), 8 deletions(-) < snip >