From: zijun_hu <zijun_hu@zoho.com> To: Mark Rutland <mark.rutland@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Laura Abbott <labbott@fedoraproject.org>, "Suzuki K. Poulose" <suzuki.poulose@arm.com>, Jeremy Linton <jeremy.linton@arm.com>, tj@kernel.org, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "stable@vger.kernel.org" <stable@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>, zijun_hu@htc.com Subject: Re: [PATCH] arm64: fix address fault during mapping fdt region Date: Mon, 1 Aug 2016 21:17:09 +0800 [thread overview] Message-ID: <579F4BD5.9060300@zoho.com> (raw) In-Reply-To: <20160801112450.GD11119@leverpostej> On 08/01/2016 07:24 PM, Mark Rutland wrote: > 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 <zijun_hu@zoho.com> 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. > okay, as indicated by another mail sent by ard.biesheuvel, "the only fields we can assume to be mapped are 'magic' and 'totalsize'", accessing last_comp_version or version maybe causes address fault under some extreme conditions according to current fdt_check_header definition, another case is regard as good fdt header, condition (fdt_magic(dt_virt) != FDT_MAGIC) don't cover as marked within below function body int fdt_check_header(const void *fdt) { if (fdt_magic(fdt) == FDT_MAGIC) { /* Complete tree */ if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; } else if (fdt_magic(fdt) == FDT_SW_MAGIC) { /* Unfinished sequential-write blob */ if (fdt_size_dt_struct(fdt) == 0) return -FDT_ERR_BADSTATE; acess field size_dt_struct too, this condition is regarded as good fdt header too } else { return -FDT_ERR_BADMAGIC; } return 0; } >> 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. > okay, the only thing my solution is depends on is the fdt header struct name which maybe remain unchanged in further fdt source modification regardless of fields layout or position or header size; by the way, my solution only maps more one SWAPPER_BLOCK_SIZE at extreme condition (offset + sizeof(struct fdt_header)) > SWAPPER_BLOCK_SIZE, it can occurs very rarely, even it happens, it is no matter due to the fast mapping operations That all said, ard.biesheuvel's can resolves address fault too, you can decide which solution to used, maybe ask fdt team for some advisements > Thanks, > Mark. >
WARNING: multiple messages have this Message-ID (diff)
From: zijun_hu@zoho.com (zijun_hu) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] arm64: fix address fault during mapping fdt region Date: Mon, 1 Aug 2016 21:17:09 +0800 [thread overview] Message-ID: <579F4BD5.9060300@zoho.com> (raw) In-Reply-To: <20160801112450.GD11119@leverpostej> On 08/01/2016 07:24 PM, Mark Rutland wrote: > 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 <zijun_hu@zoho.com> 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. > okay, as indicated by another mail sent by ard.biesheuvel, "the only fields we can assume to be mapped are 'magic' and 'totalsize'", accessing last_comp_version or version maybe causes address fault under some extreme conditions according to current fdt_check_header definition, another case is regard as good fdt header, condition (fdt_magic(dt_virt) != FDT_MAGIC) don't cover as marked within below function body int fdt_check_header(const void *fdt) { if (fdt_magic(fdt) == FDT_MAGIC) { /* Complete tree */ if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION) return -FDT_ERR_BADVERSION; } else if (fdt_magic(fdt) == FDT_SW_MAGIC) { /* Unfinished sequential-write blob */ if (fdt_size_dt_struct(fdt) == 0) return -FDT_ERR_BADSTATE; acess field size_dt_struct too, this condition is regarded as good fdt header too } else { return -FDT_ERR_BADMAGIC; } return 0; } >> 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. > okay, the only thing my solution is depends on is the fdt header struct name which maybe remain unchanged in further fdt source modification regardless of fields layout or position or header size; by the way, my solution only maps more one SWAPPER_BLOCK_SIZE at extreme condition (offset + sizeof(struct fdt_header)) > SWAPPER_BLOCK_SIZE, it can occurs very rarely, even it happens, it is no matter due to the fast mapping operations That all said, ard.biesheuvel's can resolves address fault too, you can decide which solution to used, maybe ask fdt team for some advisements > Thanks, > Mark. >
next prev parent reply other threads:[~2016-08-01 13:18 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-01 9:42 [PATCH] arm64: fix address fault during mapping fdt region zijun_hu 2016-08-01 9:42 ` zijun_hu 2016-08-01 9:50 ` Ard Biesheuvel 2016-08-01 9:50 ` Ard Biesheuvel 2016-08-01 9:50 ` Ard Biesheuvel 2016-08-01 10:59 ` zijun_hu 2016-08-01 10:59 ` zijun_hu 2016-08-01 10:59 ` zijun_hu 2016-08-01 11:24 ` Mark Rutland 2016-08-01 11:24 ` Mark Rutland 2016-08-01 11:24 ` Mark Rutland 2016-08-01 13:17 ` zijun_hu [this message] 2016-08-01 13:17 ` zijun_hu 2016-08-01 13:17 ` zijun_hu 2016-08-01 13:31 ` Mark Rutland 2016-08-01 13:31 ` Mark Rutland 2016-08-01 13:31 ` Mark Rutland 2016-08-01 11:06 ` Mark Rutland 2016-08-01 11:06 ` Mark Rutland 2016-08-01 11:06 ` Mark Rutland 2016-08-01 12:24 ` Greg KH 2016-08-01 12:24 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=579F4BD5.9060300@zoho.com \ --to=zijun_hu@zoho.com \ --cc=akpm@linux-foundation.org \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=jeremy.linton@arm.com \ --cc=labbott@fedoraproject.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=stable@vger.kernel.org \ --cc=suzuki.poulose@arm.com \ --cc=tj@kernel.org \ --cc=will.deacon@arm.com \ --cc=zijun_hu@htc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.