From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753045AbcHALZQ (ORCPT ); Mon, 1 Aug 2016 07:25:16 -0400 Received: from foss.arm.com ([217.140.101.70]:33314 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752717AbcHALZK (ORCPT ); Mon, 1 Aug 2016 07:25:10 -0400 Date: Mon, 1 Aug 2016 12:24:53 +0100 From: Mark Rutland To: zijun_hu Cc: Ard Biesheuvel , Catalin Marinas , Will Deacon , "linux-arm-kernel@lists.infradead.org" , Laura Abbott , "Suzuki K. Poulose" , Jeremy Linton , tj@kernel.org, "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Andrew Morton , zijun_hu@htc.com Subject: Re: [PATCH] arm64: fix address fault during mapping fdt region Message-ID: <20160801112450.GD11119@leverpostej> References: <579F197B.80101@zoho.com> <579F2BA6.1010203@zoho.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <579F2BA6.1010203@zoho.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 01, 2016 at 06:59:50PM +0800, zijun_hu wrote: > On 08/01/2016 05:50 PM, Ard Biesheuvel wrote: > > On 1 August 2016 at 11:42, zijun_hu wrote: > > Couldn't we simply do this instead? > this solution maybe better, my reason as follows： > > 1，it can achieve our original purpose, namely, checking whether fdt > header is corrupted before fetching fdt size field; good fdt header can > ensure good fdt size field included more rightly than only a magic filed > normally The only additional fields fdt_check_header checks are version and last_comp_version. In the absence of corruption, deferring these checks should be ok. We assume that the header is compatible regardless (or those fields would be meaningless). Even with corruption, it's possible for these to appear valid to fdt_check_header(). For the purpose of best-effort corruption detection, checking the magic alone in this early codepath seems ok to me. It seems unlikely that we'd have a valid magic yet a corrupted totalsize. That all said, I'm not against mapping the whole header if it's simple enough to do so. > 2，it is more portable; we only need to call fdt_check_header() and don't > care about fdt header filed layout; moreover,fdt module is another independent > module and arm64 only uses it and should not depend on more details of fdt > such as size and magic fields locate within the first MIN_FDT_ALIGN bytes; > the decision whether a fdt header is corrupted should be left to fdt team While it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com ([217.140.101.70]:33314 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752717AbcHALZK (ORCPT ); Mon, 1 Aug 2016 07:25:10 -0400 Date: Mon, 1 Aug 2016 12:24:53 +0100 From: Mark Rutland To: zijun_hu Cc: Ard Biesheuvel , Catalin Marinas , Will Deacon , "linux-arm-kernel@lists.infradead.org" , Laura Abbott , "Suzuki K. Poulose" , Jeremy Linton , tj@kernel.org, "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Andrew Morton , zijun_hu@htc.com Subject: Re: [PATCH] arm64: fix address fault during mapping fdt region Message-ID: <20160801112450.GD11119@leverpostej> References: <579F197B.80101@zoho.com> <579F2BA6.1010203@zoho.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <579F2BA6.1010203@zoho.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Aug 01, 2016 at 06:59:50PM +0800, zijun_hu wrote: > On 08/01/2016 05:50 PM, Ard Biesheuvel wrote: > > On 1 August 2016 at 11:42, zijun_hu wrote: > > Couldn't we simply do this instead? > this solution maybe better, my reason as follows： > > 1，it can achieve our original purpose, namely, checking whether fdt > header is corrupted before fetching fdt size field; good fdt header can > ensure good fdt size field included more rightly than only a magic filed > normally The only additional fields fdt_check_header checks are version and last_comp_version. In the absence of corruption, deferring these checks should be ok. We assume that the header is compatible regardless (or those fields would be meaningless). Even with corruption, it's possible for these to appear valid to fdt_check_header(). For the purpose of best-effort corruption detection, checking the magic alone in this early codepath seems ok to me. It seems unlikely that we'd have a valid magic yet a corrupted totalsize. That all said, I'm not against mapping the whole header if it's simple enough to do so. > 2，it is more portable; we only need to call fdt_check_header() and don't > care about fdt header filed layout; moreover,fdt module is another independent > module and arm64 only uses it and should not depend on more details of fdt > such as size and magic fields locate within the first MIN_FDT_ALIGN bytes; > the decision whether a fdt header is corrupted should be left to fdt team While it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Mon, 1 Aug 2016 12:24:53 +0100 Subject: [PATCH] arm64: fix address fault during mapping fdt region In-Reply-To: <579F2BA6.1010203@zoho.com> References: <579F197B.80101@zoho.com> <579F2BA6.1010203@zoho.com> Message-ID: <20160801112450.GD11119@leverpostej> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Aug 01, 2016 at 06:59:50PM +0800, zijun_hu wrote: > On 08/01/2016 05:50 PM, Ard Biesheuvel wrote: > > On 1 August 2016 at 11:42, zijun_hu wrote: > > Couldn't we simply do this instead? > this solution maybe better, my reason as follows? > > 1?it can achieve our original purpose, namely, checking whether fdt > header is corrupted before fetching fdt size field; good fdt header can > ensure good fdt size field included more rightly than only a magic filed > normally The only additional fields fdt_check_header checks are version and last_comp_version. In the absence of corruption, deferring these checks should be ok. We assume that the header is compatible regardless (or those fields would be meaningless). Even with corruption, it's possible for these to appear valid to fdt_check_header(). For the purpose of best-effort corruption detection, checking the magic alone in this early codepath seems ok to me. It seems unlikely that we'd have a valid magic yet a corrupted totalsize. That all said, I'm not against mapping the whole header if it's simple enough to do so. > 2?it is more portable; we only need to call fdt_check_header() and don't > care about fdt header filed layout; moreover,fdt module is another independent > module and arm64 only uses it and should not depend on more details of fdt > such as size and magic fields locate within the first MIN_FDT_ALIGN bytes; > the decision whether a fdt header is corrupted should be left to fdt team While it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format. Thanks, Mark.