From patchwork Tue Mar 14 19:23:39 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Boyd X-Patchwork-Id: 9624379 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1EFAE604CC for ; Tue, 14 Mar 2017 19:24:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 09EC0285A3 for ; Tue, 14 Mar 2017 19:24:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F3173285AF; Tue, 14 Mar 2017 19:24:11 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 31B17285A3 for ; Tue, 14 Mar 2017 19:24:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Date:Subject:Message-ID:References: In-Reply-To:From:To:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=v1r8p64U54QuG1aNQGdttqc96goco/aZGHcBwgZrv6I=; b=c/DdcDPau0EHFw UFJ9bKr/NKlXjU7YDqlU11073IJxeu40s+/7bt1mHHaxQAUHRGkZQ2Eqf3eP3STrH3HbEHmmJT+fO SmAZ/m+rLv5meKn3I5GYlEGJM03bQle4gElpjaQtQ7RmofwMmTVCPDerZc/icVI4sMg2aH4E3ILSx AYUVFuBF4w1Ci3BVk0poGRgUccUgMSg05JM5t0v2NMISBaDjOxeNue1Z2HoLmB/xM+VeozL4xfPyN 2QhMgAecAo+bYBaX4QDZZrv9a5v2Fi+4F9wZbFRRzH+Z90jbdRICtnpwXF/T1+MVMh1zcncb45Jua oiEbjo56pz4/VgBYoP0Q==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cns3G-0001kO-Rm; Tue, 14 Mar 2017 19:24:10 +0000 Received: from mail-pg0-x231.google.com ([2607:f8b0:400e:c05::231]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cns3D-0001i7-8o for linux-arm-kernel@lists.infradead.org; Tue, 14 Mar 2017 19:24:09 +0000 Received: by mail-pg0-x231.google.com with SMTP id 77so94599791pgc.1 for ; Tue, 14 Mar 2017 12:23:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:content-transfer-encoding:to:from:in-reply-to:cc :references:message-id:user-agent:subject:date; bh=Dtm88D4zMyviTxqG660Czcxjrdr77xsvNDMS2pklSMI=; b=D5VOODzTVcUNuzqpx+bkDm2IbJEO9kLzGIJl7TUjU0SJrJI9qCmge2pnb80XD5Z5i/ qxfvBkfBtKCOVdYP+4k3PvrRlvA0jKg0LyuHSQ2ZqjtInJnC012k55YGc/Kn8pjQQg4j o4vSDV5x2lwaW6YCenCVZ80pXKW9wf4FoGZxQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:content-transfer-encoding:to:from :in-reply-to:cc:references:message-id:user-agent:subject:date; bh=Dtm88D4zMyviTxqG660Czcxjrdr77xsvNDMS2pklSMI=; b=SSt+9YIQNRQMb+4MKFK9P41HTVEFEna5jkr4XdYqmku6FR8d41xbEeuG6fuwNNiCCQ VLKAZCvTP3JRmulIN+ZXxEKYxs3aBDgAcMSTBb+NsXnXEk15u8EIBdxGkQNnhcpfPZfZ 6Q+JJ7tBOudLkpKq3NBkfYE0X9lJoj1nZrm9Riahg88F7eXwq53basawWc7C0YB3Y7Vk krwGrCiLc3isy1ZIZrY8hV0iK+DudbfAaySCVT6Oc5P7BVqHz+dL1qX4RetyOJQIgapy uuu3KjNzy7rf7+Jrgb9iIqMsaVQXdt2c8+Cf+x5LrXuE5IQ7nb0NTOBmfCFPYUIsOAn6 sjqA== X-Gm-Message-State: AMke39mvonIObM2FVHWIRmXjBU0Sfai0wAdYP0Qj3BPcEMY7jQ/5rAQmG71TC7SVS8EDSXRx X-Received: by 10.98.99.196 with SMTP id x187mr46817631pfb.168.1489519425102; Tue, 14 Mar 2017 12:23:45 -0700 (PDT) Received: from linaro.org (i-global254.qualcomm.com. [199.106.103.254]) by smtp.gmail.com with ESMTPSA id e70sm40029755pfh.84.2017.03.14.12.23.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 14 Mar 2017 12:23:44 -0700 (PDT) MIME-Version: 1.0 To: Frank Rowand , Rob Herring From: Stephen Boyd In-Reply-To: <58C4EA3C.4070503@gmail.com> References: <20170214025040.23955-1-stephen.boyd@linaro.org> <58AF6B88.6020709@gmail.com> <58C4EA3C.4070503@gmail.com> Message-ID: <148951941955.30407.4217812594792852642@sboyd-linaro> User-Agent: alot/0.6.0dev Subject: Re: [RFC/PATCH] of: Mark property::value as const Date: Tue, 14 Mar 2017 12:23:39 -0700 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170314_122407_382408_7FC8F3E7 X-CRM114-Status: GOOD ( 35.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Quoting Frank Rowand (2017-03-11 22:27:08) > Hi Stephen, > > On 02/23/17 15:08, Frank Rowand wrote: > > 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. > > > > Instead of struct property field value being a pointer into the > > FDT, I would rather copy the data to newly allocated memory and > > have value be a pointer to that memory. This is required if we > > want to make /sys/firmware/fdt optional, which would allow us to > > free the memory containing the initial boot FDT. > > > > I also do not want overlay live subtrees to have any pointers > > into the FDT that was used to populate the overlay, so copying > > the data solves that problem also. > > > > > >> 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. > > > > Yes, the resolver code needs to adjust phandle values. > > > > I think I can get rid of the resolver modifying the various phandle > > values, and instead just modify the phandle value in struct > > device_node. At the same time, I think I can also remove all > > instances of the phandle properties ('linux,phandle', 'ibm,phandle', > > 'phandle') in the live tree. These properties should not be > > accessed directly by any code outside of the device tree framework > > since the phandle is located in the struct device_node. A quick > > grep does not show any such accesses of the phandle properties, > > but I want to look more closely. > > After reading through a bit of code, I am confident that I can > modify the resolver code to not modify the various phandle > property values. There are a few tentacles reaching out to > other areas that I will have to fix also. The biggest task > for me will be to create some good test code. > > I'll be unavailable this week, so I'll start on it in about > a week. Ok. No worries from my side. I'm interested to see how random properties will be "corrected" without modifying them. I can only assume the plan is to copy those properties. The kbuild robot found more cases where marking this as const causes issues. Please see the updated patch inline. -----8<----- From: Stephen Boyd Subject: [PATCH] of: Mark property::value as const 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. You can see such the problem here by loading an overlay dtb into the kernel via the fw helper method (not direct loading) and passing that tree to the resolver on an arm64 device. In this case, the firmware data is vmapped with KERNEL_PAGE_RO and the resolver code crashes when attempting to write to blob to update the phandle properties. Signed-off-by: Stephen Boyd --- drivers/of/base.c | 2 +- drivers/of/fdt.c | 12 ++++++------ drivers/of/pdt.c | 9 +++++---- drivers/of/resolver.c | 3 +++ fs/openpromfs/inode.c | 2 +- include/linux/of.h | 2 +- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/drivers/of/base.c b/drivers/of/base.c index fb6bb855714e..8e5f29b814c9 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -1156,7 +1156,7 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size); * property data is too small or too large. * */ -static void *of_find_property_value_of_size(const struct device_node *np, +static const void *of_find_property_value_of_size(const struct device_node *np, const char *propname, u32 min, u32 max, size_t *len) { struct property *prop = of_find_property(np, propname, NULL); diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index c9b5cac03b36..0635ef5dabf3 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -222,7 +222,7 @@ static void populate_properties(const void *blob, pp->name = (char *)pname; pp->length = sz; - pp->value = (__be32 *)val; + pp->value = val; *pprev = pp; pprev = &pp->next; } @@ -232,6 +232,7 @@ static void populate_properties(const void *blob, */ if (!has_name) { const char *p = nodename, *ps = p, *pa = NULL; + char *b; int len; while (*p) { @@ -250,13 +251,12 @@ static void populate_properties(const void *blob, if (!dryrun) { pp->name = "name"; pp->length = len; - pp->value = pp + 1; + pp->value = b = (char *)(pp + 1); *pprev = pp; pprev = &pp->next; - memcpy(pp->value, ps, len - 1); - ((char *)pp->value)[len - 1] = 0; - pr_debug("fixed up name for %s -> %s\n", - nodename, (char *)pp->value); + memcpy(b, ps, len - 1); + b[len - 1] = 0; + pr_debug("fixed up name for %s -> %s\n", nodename, b); } } diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c index d2acae825af9..6984717fcfb7 100644 --- a/drivers/of/pdt.c +++ b/drivers/of/pdt.c @@ -94,6 +94,7 @@ static struct property * __init of_pdt_build_one_prop(phandle node, char *prev, { static struct property *tmp = NULL; struct property *p; + char *s; int err; if (tmp) { @@ -109,8 +110,8 @@ static struct property * __init of_pdt_build_one_prop(phandle node, char *prev, if (special_name) { strcpy(p->name, special_name); p->length = special_len; - p->value = prom_early_alloc(special_len); - memcpy(p->value, special_val, special_len); + p->value = s = prom_early_alloc(special_len); + memcpy(s, special_val, special_len); } else { err = of_pdt_prom_ops->nextprop(node, prev, p->name); if (err) { @@ -123,12 +124,12 @@ static struct property * __init of_pdt_build_one_prop(phandle node, char *prev, } else { int len; - p->value = prom_early_alloc(p->length + 1); + p->value = s = prom_early_alloc(p->length + 1); len = of_pdt_prom_ops->getproperty(node, p->name, p->value, p->length); if (len <= 0) p->length = 0; - ((unsigned char *)p->value)[p->length] = '\0'; + s[p->length] = '\0'; } } return p; diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c index 8bf12e904fd2..6d88f8100318 100644 --- a/drivers/of/resolver.c +++ b/drivers/of/resolver.c @@ -93,6 +93,7 @@ static void adjust_overlay_phandles(struct device_node *overlay, if (phandle == OF_PHANDLE_ILLEGAL) continue; + /* This is bad because we cast away const */ *(uint32_t *)prop->value = cpu_to_be32(overlay->phandle); } @@ -154,6 +155,7 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay, goto err_fail; } + /* This is bad because we cast away const */ *(__be32 *)(prop->value + offset) = cpu_to_be32(phandle); } @@ -222,6 +224,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups, phandle = be32_to_cpu(*(__be32 *)(prop->value + off)); phandle += phandle_delta; + /* This is bad because we cast away const */ *(__be32 *)(prop->value + off) = cpu_to_be32(phandle); } } diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c index 13215f26e321..68cbb3fddbc2 100644 --- a/fs/openpromfs/inode.c +++ b/fs/openpromfs/inode.c @@ -65,7 +65,7 @@ static int is_string(unsigned char *p, int len) static int property_show(struct seq_file *f, void *v) { struct property *prop = f->private; - void *pval; + const void *pval; int len; len = prop->length; diff --git a/include/linux/of.h b/include/linux/of.h index f22d4a83ca07..b0253886ef46 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -35,7 +35,7 @@ typedef u32 ihandle; struct property { char *name; int length; - void *value; + const void *value; struct property *next; unsigned long _flags; unsigned int unique_id;