From: loic pallardy <loic.pallardy@st.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>,
Lee Jones <lee.jones@linaro.org>
Cc: ohad@wizery.com, kernel@stlinux.com,
linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big
Date: Fri, 17 Jun 2016 21:53:08 +0200 [thread overview]
Message-ID: <57645524.1030200@st.com> (raw)
In-Reply-To: <20160510192129.GL1256@tuxbot>
Hi,
On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary. However,
>> DT can over-ride this by providing a different (smaller) size. We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.
Agree with Bjorn, not it is not possible to silently clip carveout
memory.Firmware resource table should contain exact coprocessor needs.
If resources are not available, firmware loading must failed with
explicit message.
Regards,
Loic
>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>> return -ENOMEM;
>>
>> dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> + sub = dev_get_drvdata(dma_dev);
>> +
>> + if (rsc->len > resource_size(sub->res)) {
>> + dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> + rsc->len, resource_size(sub->res));
>> + rsc->len = resource_size(sub->res);
>> + }
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> dev_err(dev, "defined carveout to small for firmware\n");
> return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>> va = dma_alloc_coherent(dma_dev, rsc->len, &dma, GFP_KERNEL);
>> if (!va) {
>> dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>
WARNING: multiple messages have this Message-ID (diff)
From: loic pallardy <loic.pallardy@st.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>,
Lee Jones <lee.jones@linaro.org>
Cc: <ohad@wizery.com>, <kernel@stlinux.com>,
<linux-remoteproc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big
Date: Fri, 17 Jun 2016 21:53:08 +0200 [thread overview]
Message-ID: <57645524.1030200@st.com> (raw)
In-Reply-To: <20160510192129.GL1256@tuxbot>
Hi,
On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary. However,
>> DT can over-ride this by providing a different (smaller) size. We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.
Agree with Bjorn, not it is not possible to silently clip carveout
memory.Firmware resource table should contain exact coprocessor needs.
If resources are not available, firmware loading must failed with
explicit message.
Regards,
Loic
>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>> return -ENOMEM;
>>
>> dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> + sub = dev_get_drvdata(dma_dev);
>> +
>> + if (rsc->len > resource_size(sub->res)) {
>> + dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> + rsc->len, resource_size(sub->res));
>> + rsc->len = resource_size(sub->res);
>> + }
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> dev_err(dev, "defined carveout to small for firmware\n");
> return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>> va = dma_alloc_coherent(dma_dev, rsc->len, &dma, GFP_KERNEL);
>> if (!va) {
>> dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>
WARNING: multiple messages have this Message-ID (diff)
From: loic.pallardy@st.com (loic pallardy)
To: linux-arm-kernel@lists.infradead.org
Subject: [STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big
Date: Fri, 17 Jun 2016 21:53:08 +0200 [thread overview]
Message-ID: <57645524.1030200@st.com> (raw)
In-Reply-To: <20160510192129.GL1256@tuxbot>
Hi,
On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary. However,
>> DT can over-ride this by providing a different (smaller) size. We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.
Agree with Bjorn, not it is not possible to silently clip carveout
memory.Firmware resource table should contain exact coprocessor needs.
If resources are not available, firmware loading must failed with
explicit message.
Regards,
Loic
>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>> return -ENOMEM;
>>
>> dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> + sub = dev_get_drvdata(dma_dev);
>> +
>> + if (rsc->len > resource_size(sub->res)) {
>> + dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> + rsc->len, resource_size(sub->res));
>> + rsc->len = resource_size(sub->res);
>> + }
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> dev_err(dev, "defined carveout to small for firmware\n");
> return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>> va = dma_alloc_coherent(dma_dev, rsc->len, &dma, GFP_KERNEL);
>> if (!va) {
>> dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel at stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>
next prev parent reply other threads:[~2016-06-17 19:53 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-05 13:29 [PATCH 0/5] remoteproc: A few important improvements Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-05 13:29 ` [PATCH 1/5] remoteproc: core: Task sync during rproc_fw_boot() Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-06 18:44 ` Bjorn Andersson
2016-05-06 18:44 ` Bjorn Andersson
2016-05-05 13:29 ` [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-06 18:48 ` Bjorn Andersson
2016-05-06 18:48 ` Bjorn Andersson
2016-05-10 14:16 ` Lee Jones
2016-05-10 14:16 ` Lee Jones
2016-05-10 18:48 ` Bjorn Andersson
2016-05-10 18:48 ` Bjorn Andersson
2016-07-13 19:11 ` Bjorn Andersson
2016-07-13 19:11 ` Bjorn Andersson
2016-07-14 6:53 ` Lee Jones
2016-07-14 6:53 ` Lee Jones
2016-05-05 13:29 ` [PATCH 3/5] remoteproc: core: Add ability to select a firmware from the client Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-06 18:59 ` Bjorn Andersson
2016-05-06 18:59 ` Bjorn Andersson
2016-05-10 13:02 ` Lee Jones
2016-05-10 13:02 ` Lee Jones
2016-05-05 13:29 ` [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-11 22:30 ` Bjorn Andersson
2016-05-11 22:30 ` Bjorn Andersson
2016-06-15 22:06 ` Bjorn Andersson
2016-06-15 22:06 ` Bjorn Andersson
2016-06-21 7:33 ` [STLinux Kernel] " loic pallardy
2016-06-21 7:33 ` loic pallardy
2016-06-21 7:33 ` loic pallardy
2016-06-22 16:21 ` Bjorn Andersson
2016-06-22 16:21 ` Bjorn Andersson
2016-05-05 13:29 ` [PATCH 5/5] remoteproc: core: Clip carveout if it's too big Lee Jones
2016-05-05 13:29 ` Lee Jones
2016-05-10 19:21 ` Bjorn Andersson
2016-05-10 19:21 ` Bjorn Andersson
2016-06-17 19:53 ` loic pallardy [this message]
2016-06-17 19:53 ` [STLinux Kernel] " loic pallardy
2016-06-17 19:53 ` loic pallardy
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=57645524.1030200@st.com \
--to=loic.pallardy@st.com \
--cc=bjorn.andersson@linaro.org \
--cc=kernel@stlinux.com \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=ohad@wizery.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: link
Be 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.