[PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Hello.

Jon Nettleton made me aware that there is a driver which makes it very
easy to write to the OTP from userspace.

And I have to admit that I was mildly shocked.

Because OTP stands for One Time Programmable, it means the write
functionality of that driver can be used only once per HW and if that
is done wrong, the HW might be one of those famous electronic bricks
afterwards.

So I've quickly written a patch which makes this driver to a friendly
citizen instead of an heavily armed cowboy.

I really, really think this patch should be attached to all trees which
are including fsl_otp.c. It applies without any error to
imx_3.10.17_1.0.1_ga, imx_3.10.31_1.1.0_beta, linux-linaro-lsk-v3.14-mx6
and likely any other tree which includes that driver.

Regards,

Alexander Holler

[PATCH 1/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Nothing in userspace should be able to kill the HW.
Not even just as root and for sure not that easy.

For obvious reason, I haven't tested this patch thoroughly.

Reported-by: Jon Nettleton <jon.nettleton@gmail.com>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/char/fsl_otp.c | 57 +-------------------------------------------------
 1 file changed, 1 insertion(+), 56 deletions(-)

diff --git a/drivers/char/fsl_otp.c b/drivers/char/fsl_otp.c
index face297..d9d1c59 100644
--- a/drivers/char/fsl_otp.c
+++ b/drivers/char/fsl_otp.c
@@ -151,60 +151,6 @@ out:
 	return ret ? 0 : sprintf(buf, "0x%x\n", value);
 }
 
-static int otp_write_bits(int addr, u32 data, u32 magic)
-{
-	u32 c; /* for control register */
-
-	/* init the control register */
-	c = __raw_readl(otp_base + HW_OCOTP_CTRL);
-	c &= ~BM_OCOTP_CTRL_ADDR;
-	c |= BF(addr, OCOTP_CTRL_ADDR);
-	c |= BF(magic, OCOTP_CTRL_WR_UNLOCK);
-	__raw_writel(c, otp_base + HW_OCOTP_CTRL);
-
-	/* init the data register */
-	__raw_writel(data, otp_base + HW_OCOTP_DATA);
-	otp_wait_busy(0);
-
-	mdelay(2); /* Write Postamble */
-
-	return 0;
-}
-
-static ssize_t fsl_otp_store(struct kobject *kobj, struct kobj_attribute *attr,
-			     const char *buf, size_t count)
-{
-	unsigned int index = attr - otp_kattr;
-	u32 value;
-	int ret;
-
-	sscanf(buf, "0x%x", &value);
-
-	ret = clk_prepare_enable(otp_clk);
-	if (ret)
-		return 0;
-
-	mutex_lock(&otp_mutex);
-
-	set_otp_timing();
-	ret = otp_wait_busy(0);
-	if (ret)
-		goto out;
-
-	otp_write_bits(index, value, 0x3e77);
-
-	/* Reload all the shadow registers */
-	__raw_writel(BM_OCOTP_CTRL_RELOAD_SHADOWS,
-		     otp_base + HW_OCOTP_CTRL_SET);
-	udelay(1);
-	otp_wait_busy(BM_OCOTP_CTRL_RELOAD_SHADOWS);
-
-out:
-	mutex_unlock(&otp_mutex);
-	clk_disable_unprepare(otp_clk);
-	return ret ? 0 : count;
-}
-
 static int fsl_otp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -244,9 +190,8 @@ static int fsl_otp_probe(struct platform_device *pdev)
 	for (i = 0; i < num; i++) {
 		sysfs_attr_init(&otp_kattr[i].attr);
 		otp_kattr[i].attr.name = desc[i];
-		otp_kattr[i].attr.mode = 0600;
+		otp_kattr[i].attr.mode = 0400;
 		otp_kattr[i].show = fsl_otp_show;
-		otp_kattr[i].store = fsl_otp_store;
 		attrs[i] = &otp_kattr[i].attr;
 	}
 	otp_attr_group->attrs = attrs;
-- 
1.8.3.1

Re: [PATCH 1/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Otavio Salvador]

On Fri, Nov 7, 2014 at 7:43 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> Nothing in userspace should be able to kill the HW.
> Not even just as root and for sure not that easy.
>
> For obvious reason, I haven't tested this patch thoroughly.
>
> Reported-by: Jon Nettleton <jon.nettleton@gmail.com>
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>

I have a mix of feelings about the patch.

Long story, short:

 - We ought to have a FSL_OTP_WRITE_SUPPORT config option

The writing support is specially useful for manufacturing images where
we can use Linux to do the initial programing, consult a database to
find out inventory information (MAC address for example) and do proper
programing on the OTP fuses. However I also believe we shouldn't have
this available on regular kernel images as it is easy to get a
'expensive paper weight'.

So would you be keen to rework the patch and include a write support
config option?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH 1/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 07.11.2014 um 12:34 schrieb Otavio Salvador:
> On Fri, Nov 7, 2014 at 7:43 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Nothing in userspace should be able to kill the HW.
>> Not even just as root and for sure not that easy.
>>
>> For obvious reason, I haven't tested this patch thoroughly.
>>
>> Reported-by: Jon Nettleton <jon.nettleton@gmail.com>
>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>
> I have a mix of feelings about the patch.
>
> Long story, short:
>
>  - We ought to have a FSL_OTP_WRITE_SUPPORT config option
>
> The writing support is specially useful for manufacturing images where
> we can use Linux to do the initial programing, consult a database to
> find out inventory information (MAC address for example) and do proper
> programing on the OTP fuses. However I also believe we shouldn't have
> this available on regular kernel images as it is easy to get a
> 'expensive paper weight'.
>
> So would you be keen to rework the patch and include a write support
> config option?

No.

Sorry but I really think such dangerous stuff never should make it's way
into any kernel which doesn't have big red and yellow signs attached to
it. And unfortunately that isn't possible.

And even if it would be possible to mark kernels as dangerous, I think
that functionality should only be part of a bootloader where it isn't 
reachable by normal userspace.

Regards,

Alexander Holler

Re: [PATCH 1/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Otavio Salvador]

On Fri, Nov 7, 2014 at 12:40 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 07.11.2014 um 12:34 schrieb Otavio Salvador:
>>
>> On Fri, Nov 7, 2014 at 7:43 AM, Alexander Holler <holler@ahsoftware.de>
>> wrote:
>>>
>>> Nothing in userspace should be able to kill the HW.
>>> Not even just as root and for sure not that easy.
>>>
>>> For obvious reason, I haven't tested this patch thoroughly.
>>>
>>> Reported-by: Jon Nettleton <jon.nettleton@gmail.com>
>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>>
>>
>> I have a mix of feelings about the patch.
>>
>> Long story, short:
>>
>>  - We ought to have a FSL_OTP_WRITE_SUPPORT config option
>>
>> The writing support is specially useful for manufacturing images where
>> we can use Linux to do the initial programing, consult a database to
>> find out inventory information (MAC address for example) and do proper
>> programing on the OTP fuses. However I also believe we shouldn't have
>> this available on regular kernel images as it is easy to get a
>> 'expensive paper weight'.
>>
>> So would you be keen to rework the patch and include a write support
>> config option?
>
>
> No.
>
> Sorry but I really think such dangerous stuff never should make it's way
> into any kernel which doesn't have big red and yellow signs attached to
> it. And unfortunately that isn't possible.
>
> And even if it would be possible to mark kernels as dangerous, I think
> that functionality should only be part of a bootloader where it isn't
> reachable by normal userspace.

Ok so I am here Nacking your patch.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH 1/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 07.11.2014 16:06, schrieb Otavio Salvador:
> On Fri, Nov 7, 2014 at 12:40 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 07.11.2014 um 12:34 schrieb Otavio Salvador:
>>>
>>> On Fri, Nov 7, 2014 at 7:43 AM, Alexander Holler <holler@ahsoftware.de>
>>> wrote:
>>>>
>>>> Nothing in userspace should be able to kill the HW.
>>>> Not even just as root and for sure not that easy.
>>>>
>>>> For obvious reason, I haven't tested this patch thoroughly.
>>>>
>>>> Reported-by: Jon Nettleton <jon.nettleton@gmail.com>
>>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>>>
>>>
>>> I have a mix of feelings about the patch.
>>>
>>> Long story, short:
>>>
>>>   - We ought to have a FSL_OTP_WRITE_SUPPORT config option
>>>
>>> The writing support is specially useful for manufacturing images where
>>> we can use Linux to do the initial programing, consult a database to
>>> find out inventory information (MAC address for example) and do proper
>>> programing on the OTP fuses. However I also believe we shouldn't have
>>> this available on regular kernel images as it is easy to get a
>>> 'expensive paper weight'.
>>>
>>> So would you be keen to rework the patch and include a write support
>>> config option?
>>
>>
>> No.
>>
>> Sorry but I really think such dangerous stuff never should make it's way
>> into any kernel which doesn't have big red and yellow signs attached to
>> it. And unfortunately that isn't possible.
>>
>> And even if it would be possible to mark kernels as dangerous, I think
>> that functionality should only be part of a bootloader where it isn't
>> reachable by normal userspace.
>
> Ok so I am here Nacking your patch.

Feel free to so. Seems to be part of your responsibility.

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Eric Bénard]

Hi Alexander,

Le Fri,  7 Nov 2014 10:43:34 +0100,
Alexander Holler <holler@ahsoftware.de> a écrit :
> Jon Nettleton made me aware that there is a driver which makes it very
> easy to write to the OTP from userspace.
> 
> And I have to admit that I was mildly shocked.
> 
> Because OTP stands for One Time Programmable, it means the write
> functionality of that driver can be used only once per HW and if that
> is done wrong, the HW might be one of those famous electronic bricks
> afterwards.
> 
> So I've quickly written a patch which makes this driver to a friendly
> citizen instead of an heavily armed cowboy.
> 
> I really, really think this patch should be attached to all trees which
> are including fsl_otp.c. It applies without any error to
> imx_3.10.17_1.0.1_ga, imx_3.10.31_1.1.0_beta, linux-linaro-lsk-v3.14-mx6
> and likely any other tree which includes that driver.
> 
That's not shocking to have this feature in a reference design's BSP : 
Freescale provides base source code to evaluate their SOC and writing
OTP is one feature of their SOC.
Then that's the end product designer's responsibility to enable/disable
features he wants or not on its product.

The solution proposed by Otavio to add an option to ease the
disabling of this feature is far better than fully removing the code.

Best regards,
Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Jon Nettleton]

Fair enough.  Can we all agree that it should default to disabled?

-Jon

On Fri, Nov 7, 2014 at 3:00 PM, Eric Bénard <eric@eukrea.com> wrote:
> Hi Alexander,
>
> Le Fri,  7 Nov 2014 10:43:34 +0100,
> Alexander Holler <holler@ahsoftware.de> a écrit :
>> Jon Nettleton made me aware that there is a driver which makes it very
>> easy to write to the OTP from userspace.
>>
>> And I have to admit that I was mildly shocked.
>>
>> Because OTP stands for One Time Programmable, it means the write
>> functionality of that driver can be used only once per HW and if that
>> is done wrong, the HW might be one of those famous electronic bricks
>> afterwards.
>>
>> So I've quickly written a patch which makes this driver to a friendly
>> citizen instead of an heavily armed cowboy.
>>
>> I really, really think this patch should be attached to all trees which
>> are including fsl_otp.c. It applies without any error to
>> imx_3.10.17_1.0.1_ga, imx_3.10.31_1.1.0_beta, linux-linaro-lsk-v3.14-mx6
>> and likely any other tree which includes that driver.
>>
> That's not shocking to have this feature in a reference design's BSP :
> Freescale provides base source code to evaluate their SOC and writing
> OTP is one feature of their SOC.
> Then that's the end product designer's responsibility to enable/disable
> features he wants or not on its product.
>
> The solution proposed by Otavio to add an option to ease the
> disabling of this feature is far better than fully removing the code.
>
> Best regards,
> Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 07.11.2014 15:31, schrieb Jon Nettleton:
> Fair enough.  Can we all agree that it should default to disabled?

Anyone who is in need to write the OTP should be able to apply a patch 
or to do a git revert in order to use that functionality.

Those which are unable to do so, might not have an idea about OTP at 
all, or they might not be aware how dangerous it is to write it (besides 
the necessary knowledge about what to change).

Regards,

Alexander Holler

>
> -Jon
>
> On Fri, Nov 7, 2014 at 3:00 PM, Eric Bénard <eric@eukrea.com> wrote:
>> Hi Alexander,
>>
>> Le Fri,  7 Nov 2014 10:43:34 +0100,
>> Alexander Holler <holler@ahsoftware.de> a écrit :
>>> Jon Nettleton made me aware that there is a driver which makes it very
>>> easy to write to the OTP from userspace.
>>>
>>> And I have to admit that I was mildly shocked.
>>>
>>> Because OTP stands for One Time Programmable, it means the write
>>> functionality of that driver can be used only once per HW and if that
>>> is done wrong, the HW might be one of those famous electronic bricks
>>> afterwards.
>>>
>>> So I've quickly written a patch which makes this driver to a friendly
>>> citizen instead of an heavily armed cowboy.
>>>
>>> I really, really think this patch should be attached to all trees which
>>> are including fsl_otp.c. It applies without any error to
>>> imx_3.10.17_1.0.1_ga, imx_3.10.31_1.1.0_beta, linux-linaro-lsk-v3.14-mx6
>>> and likely any other tree which includes that driver.
>>>
>> That's not shocking to have this feature in a reference design's BSP :
>> Freescale provides base source code to evaluate their SOC and writing
>> OTP is one feature of their SOC.
>> Then that's the end product designer's responsibility to enable/disable
>> features he wants or not on its product.
>>
>> The solution proposed by Otavio to add an option to ease the
>> disabling of this feature is far better than fully removing the code.
>>
>> Best regards,
>> Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Eric Bénard]

Hi Jon,

Le Fri, 7 Nov 2014 15:31:15 +0100,
Jon Nettleton <jon.nettleton@gmail.com> a écrit :
> Fair enough.  Can we all agree that it should default to disabled?

On your product sure, on the evaluation board that's more questionable.

When an evaluation board's user reach this sysfs entries to do a write
to it I believe that's not a random choice but a user's choice.

Maybe adding a otp_unlock entry to the sysfs before allowing write could
secure more the feature when running on the evaluation boards so that
the user can think twice before writing to the fuses.

Best regards,
Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Otavio Salvador]

On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
> Hi Jon,
>
> Le Fri, 7 Nov 2014 15:31:15 +0100,
> Jon Nettleton <jon.nettleton@gmail.com> a écrit :
>> Fair enough.  Can we all agree that it should default to disabled?
>
> On your product sure, on the evaluation board that's more questionable.
>
> When an evaluation board's user reach this sysfs entries to do a write
> to it I believe that's not a random choice but a user's choice.
>
> Maybe adding a otp_unlock entry to the sysfs before allowing write could
> secure more the feature when running on the evaluation boards so that
> the user can think twice before writing to the fuses.

I like Eric's idea of the unlock sysfs entry for this purpose.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 07.11.2014 16:07, schrieb Otavio Salvador:
> On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
>> Hi Jon,
>>
>> Le Fri, 7 Nov 2014 15:31:15 +0100,
>> Jon Nettleton <jon.nettleton@gmail.com> a écrit :
>>> Fair enough.  Can we all agree that it should default to disabled?
>>
>> On your product sure, on the evaluation board that's more questionable.
>>
>> When an evaluation board's user reach this sysfs entries to do a write
>> to it I believe that's not a random choice but a user's choice.
>>
>> Maybe adding a otp_unlock entry to the sysfs before allowing write could
>> secure more the feature when running on the evaluation boards so that
>> the user can think twice before writing to the fuses.
>
> I like Eric's idea of the unlock sysfs entry for this purpose.

Maybe I should mention that this extremly dangerous driver already was 
posted for submission into the (production) kernel and nobody had a 
problem with it's write functionality, just with some other stuff.

And even if it's ok for FSL to promote this feature with their BSP, I 
have absolutely no love for the way it was done. It now is likely part 
of many production kernels, something never should have happened.

And I still see absolutely no need to have some one time functionality 
as part of the kernel, regardless how usefull it might be for some 
developers. It's a waste of time and resources to include that write 
functionality in something meant for the greater public. Especially 
something that dangerous.

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Otavio Salvador]

Hello Alexander,

On Fri, Nov 7, 2014 at 1:23 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 07.11.2014 16:07, schrieb Otavio Salvador:
>>
>> On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
>>>
>>> Hi Jon,
>>>
>>> Le Fri, 7 Nov 2014 15:31:15 +0100,
>>> Jon Nettleton <jon.nettleton@gmail.com> a écrit :
>>>>
>>>> Fair enough.  Can we all agree that it should default to disabled?
>>>
>>>
>>> On your product sure, on the evaluation board that's more questionable.
>>>
>>> When an evaluation board's user reach this sysfs entries to do a write
>>> to it I believe that's not a random choice but a user's choice.
>>>
>>> Maybe adding a otp_unlock entry to the sysfs before allowing write could
>>> secure more the feature when running on the evaluation boards so that
>>> the user can think twice before writing to the fuses.
>>
>>
>> I like Eric's idea of the unlock sysfs entry for this purpose.
>
>
> Maybe I should mention that this extremly dangerous driver already was
> posted for submission into the (production) kernel and nobody had a problem
> with it's write functionality, just with some other stuff.
>
> And even if it's ok for FSL to promote this feature with their BSP, I have
> absolutely no love for the way it was done. It now is likely part of many
> production kernels, something never should have happened.
>
> And I still see absolutely no need to have some one time functionality as
> part of the kernel, regardless how usefull it might be for some developers.
> It's a waste of time and resources to include that write functionality in
> something meant for the greater public. Especially something that dangerous.

We have two different things here.

 * Linux support: the driver is perfectly capable of handling the
reading and writing
 * Driver settings: We should, indeed, have it to be explicitly
enabled on the kernel config to allow writing

This is done in other areas of kernel (one which comes to my mind is
NTFS Writing support) and what I proposed was you to make:

CONFIG_FSL_OTP -> read-only
CONFIG_FSL_OTP_WRITE -> write support

The write support needs to be enabled on mfgtools defconfig and
disabled on regular kernels (this is what I think it should be but it
is up to the board maintainer).

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 07.11.2014 17:00, schrieb Otavio Salvador:
> Hello Alexander,
>
> On Fri, Nov 7, 2014 at 1:23 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 07.11.2014 16:07, schrieb Otavio Salvador:
>>>
>>> On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
>>>>
>>>> Hi Jon,
>>>>
>>>> Le Fri, 7 Nov 2014 15:31:15 +0100,
>>>> Jon Nettleton <jon.nettleton@gmail.com> a écrit :
>>>>>
>>>>> Fair enough.  Can we all agree that it should default to disabled?
>>>>
>>>>
>>>> On your product sure, on the evaluation board that's more questionable.
>>>>
>>>> When an evaluation board's user reach this sysfs entries to do a write
>>>> to it I believe that's not a random choice but a user's choice.
>>>>
>>>> Maybe adding a otp_unlock entry to the sysfs before allowing write could
>>>> secure more the feature when running on the evaluation boards so that
>>>> the user can think twice before writing to the fuses.
>>>
>>>
>>> I like Eric's idea of the unlock sysfs entry for this purpose.
>>
>>
>> Maybe I should mention that this extremly dangerous driver already was
>> posted for submission into the (production) kernel and nobody had a problem
>> with it's write functionality, just with some other stuff.
>>
>> And even if it's ok for FSL to promote this feature with their BSP, I have
>> absolutely no love for the way it was done. It now is likely part of many
>> production kernels, something never should have happened.
>>
>> And I still see absolutely no need to have some one time functionality as
>> part of the kernel, regardless how usefull it might be for some developers.
>> It's a waste of time and resources to include that write functionality in
>> something meant for the greater public. Especially something that dangerous.
>
> We have two different things here.
>
>   * Linux support: the driver is perfectly capable of handling the
> reading and writing
>   * Driver settings: We should, indeed, have it to be explicitly
> enabled on the kernel config to allow writing
>
> This is done in other areas of kernel (one which comes to my mind is
> NTFS Writing support) and what I proposed was you to make:
>
> CONFIG_FSL_OTP -> read-only
> CONFIG_FSL_OTP_WRITE -> write support
>
> The write support needs to be enabled on mfgtools defconfig and
> disabled on regular kernels (this is what I think it should be but it
> is up to the board maintainer).

Sorry, but based on my experience and confrontations with Murphy I can't 
agree.

To conclude what already happened:

- Someone has written that driver in the way it is.
- Several devs likely have seen it before it made it's way into the BSP.
- It got enabled by default in the BSP (defconfigs).
- It made it's way into YOCTO.
- It was submitted for inclusion into the kernel.

So the driver already was seen by a lot of people (most likely all devs) 
and still nobody has seen a problem. And most of these people did know 
more about its functionality than most people which are just changing 
switches in kernel configs or do play with sysfs.

And because I don't like to play the bad boy, I'm not going to discuss 
this any further. I did my part as responsible developer and I think I 
made my standpoint clear. I hoped it would have happened more silently 
without that discussion, but ... ;)

But feel free to continue this discussion, I don't care if my patch will 
be included or not. I just will not modify it. ;)

Regards,

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Nikolay Dimitrov]

Hi Alexander,

The driver allows to be enabled/disabled by a configuration option, so
it's a responsibility of your engineering team to properly configure
the software for development, manufacturing and production purposes, as
there's no "one size fits all" solution for this option.

And I think it would be a unwise decision just to cripple the driver
because there is potential for misuse the driver. If so, we have to
disable also all device files and sysfs entries that allow raw access
to physical memory, physical disks, cpu frequency, thermal device,
power supply voltages, as all of these can screw-up the product (either
by deleting data or by frying a component on the board). And we have to
forbid kitchen knives :).

I like Eric's idea with sysfs unlock entry. It's also possible to have
different sysfs "read" and "write" files' permissions, to provide
privilege separation.

I also understand your confusion of the answers received on LKML and
here, but you should already know that each FOSS tribe has its own
customs. What's good for the kernel itself doesn't make it instantly
good for actual systems/product integration, so it's normal to have
groups with different goals to react differently on the same topic.

Kind regards,
Nikolay

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Chris Tapp]

On 8 Nov 2014, at 02:03, Nikolay Dimitrov <picmaster@mail.bg> wrote:

> Hi Alexander,
> 
> The driver allows to be enabled/disabled by a configuration option, so
> it's a responsibility of your engineering team to properly configure
> the software for development, manufacturing and production purposes, as
> there's no "one size fits all" solution for this option.
> 
> And I think it would be a unwise decision just to cripple the driver
> because there is potential for misuse the driver. If so, we have to
> disable also all device files and sysfs entries that allow raw access
> to physical memory, physical disks, cpu frequency, thermal device,
> power supply voltages, as all of these can screw-up the product (either
> by deleting data or by frying a component on the board). And we have to
> forbid kitchen knives :).
> 
> I like Eric's idea with sysfs unlock entry. It's also possible to have
> different sysfs "read" and "write" files' permissions, to provide
> privilege separation.
> 
> I also understand your confusion of the answers received on LKML and
> here, but you should already know that each FOSS tribe has its own
> customs. What's good for the kernel itself doesn't make it instantly
> good for actual systems/product integration, so it's normal to have
> groups with different goals to react differently on the same topic.

As has already been mentioned, it is the responsibility of a project to ensure that any product is suitably protected.

I can image a customer would be really unhappy if their system (e.g. Smart TV) could be destroyed by a bit of malware !

--

Chris Tapp
opensource@keylevel.com
www.keylevel.com

----
You can tell you're getting older when your car insurance gets real cheap!

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Jon Nettleton]

On Sat, Nov 8, 2014 at 9:58 AM, Chris Tapp <opensource@keylevel.com> wrote:
>
> On 8 Nov 2014, at 02:03, Nikolay Dimitrov <picmaster@mail.bg> wrote:
>
>> Hi Alexander,
>>
>> The driver allows to be enabled/disabled by a configuration option, so
>> it's a responsibility of your engineering team to properly configure
>> the software for development, manufacturing and production purposes, as
>> there's no "one size fits all" solution for this option.
>>
>> And I think it would be a unwise decision just to cripple the driver
>> because there is potential for misuse the driver. If so, we have to
>> disable also all device files and sysfs entries that allow raw access
>> to physical memory, physical disks, cpu frequency, thermal device,
>> power supply voltages, as all of these can screw-up the product (either
>> by deleting data or by frying a component on the board). And we have to
>> forbid kitchen knives :).
>>
>> I like Eric's idea with sysfs unlock entry. It's also possible to have
>> different sysfs "read" and "write" files' permissions, to provide
>> privilege separation.
>>
>> I also understand your confusion of the answers received on LKML and
>> here, but you should already know that each FOSS tribe has its own
>> customs. What's good for the kernel itself doesn't make it instantly
>> good for actual systems/product integration, so it's normal to have
>> groups with different goals to react differently on the same topic.
>
> As has already been mentioned, it is the responsibility of a project to ensure that any product is suitably protected.
>
> I can image a customer would be really unhappy if their system (e.g. Smart TV) could be destroyed by a bit of malware !
>

Yes but many of the boards aren't necessarily being used for locked
down consumer products.  Many people are using these devices as
learning devices and are trusting the code provided by the
manufacturer.  I think it makes sense for public software to try and
provide the most sane configuration.  It is trivial to make this a
kernel config option that is disabled by default, add a write_enabled
sysfs node, and even have it spit out a warning when you enable the
write_enabled sysfs node.  If we do all those things and somebody
bricks their board then I would not have a bad conscience.

Anybody that has the skill or reason to use the fuses properly will
have no problem following steps to enable this functionality.

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 08.11.2014 03:03, schrieb Nikolay Dimitrov:
> Hi Alexander,
>
> The driver allows to be enabled/disabled by a configuration option, so
> it's a responsibility of your engineering team to properly configure
> the software for development, manufacturing and production purposes, as
> there's no "one size fits all" solution for this option.

Sorry to disappoint you, but I'm currently just a hobbyist. ;)

> And I think it would be a unwise decision just to cripple the driver
> because there is potential for misuse the driver. If so, we have to
> disable also all device files and sysfs entries that allow raw access
> to physical memory, physical disks, cpu frequency, thermal device,
> power supply voltages, as all of these can screw-up the product (either
> by deleting data or by frying a component on the board). And we have to
> forbid kitchen knives :).

Hmm, maybe I should repeat that this driver is about ONE TIME 
PROGRAMMABLE memory. So all your comparisons are (imho) totally wrong.

YOU ONLY HAVE ONE SHOT to set one of these values to whatever value you 
want. And if you get it wrong, the HW might be unusable afterwards. Not 
to speak about all the things which can go wrong and might write to one 
of these files, changing the (hopefully correct) values such that the HW 
is dead afterwards too.

> I like Eric's idea with sysfs unlock entry. It's also possible to have
> different sysfs "read" and "write" files' permissions, to provide
> privilege separation.
>
> I also understand your confusion of the answers received on LKML and
> here, but you should already know that each FOSS tribe has its own
> customs. What's good for the kernel itself doesn't make it instantly
> good for actual systems/product integration, so it's normal to have
> groups with different goals to react differently on the same topic.

I'm not confused, at least not in regard what you want to suggest. Of 
course, I'm totally confused about the fact that almost nobody else 
before has critized that write functionality of this driver, also I'm 
already used to the fact that I'm unable to understand many things which 
are happing in todays world. But nobody is perfect. ;)

But there is absolutely no reason to include this ONE TIME FUNCTIONALITY 
into any kernel meant for the public, especially as it is very 
dangerous. And for sure not without any content veryfication and some 
other means to make sure the value one wants to put ONCE into that ONE 
TIME MEMORY isn't wrong.

That's why I just refuse to change my patch in order to add write 
functionality. I don't want to do (imho) foolish things just because 
someone else tells me to do them. I'm no Lemming.

At least not without adequate compensations. ;)

Regards,

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 08.11.2014 19:49, schrieb Alexander Holler:

> I'm not confused, at least not in regard what you want to suggest. Of
> course, I'm totally confused about the fact that almost nobody else
> before has critized that write functionality of this driver, also I'm
> already used to the fact that I'm unable to understand many things which
> are happing in todays world. But nobody is perfect. ;)

Just to make it more clear what this thread is about, here is a relevant 
sentence copied form the reference manual for the chip:

"In order to avoid "rogue" code performing erroneous writes to OTP, a 
special unlocking sequence is required for writes to the fuse banks."

Now guess why the HW was designed this way. And then look again at the 
driver which nullifies the careful implementation in the HW. It doesn't 
have to be the fault of the author, e.g. he just might have written it 
for internal use. The problem is that it went into kernels for public 
use and nobody has seen a problem. Might be because of missing knowledge 
about what the driver does at all or whatever. I don't know.

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Eric Bénard]

Hi Alexander,

Le Sun, 09 Nov 2014 11:14:39 +0100,
Alexander Holler <holler@ahsoftware.de> a écrit :

> Am 08.11.2014 19:49, schrieb Alexander Holler:
> 
> > I'm not confused, at least not in regard what you want to suggest. Of
> > course, I'm totally confused about the fact that almost nobody else
> > before has critized that write functionality of this driver, also I'm
> > already used to the fact that I'm unable to understand many things which
> > are happing in todays world. But nobody is perfect. ;)
> 
> Just to make it more clear what this thread is about, here is a relevant 
> sentence copied form the reference manual for the chip:
> 
> "In order to avoid "rogue" code performing erroneous writes to OTP, a 
> special unlocking sequence is required for writes to the fuse banks."
> 
That's why adding an unlock sysfs entry would match the required
sequence to unlock the write access on the user point of view, but
that's Freescale's problem and policy.

> Now guess why the HW was designed this way. And then look again at the 
> driver which nullifies the careful implementation in the HW. It doesn't 
> have to be the fault of the author, e.g. he just might have written it 
> for internal use. The problem is that it went into kernels for public 
> use and nobody has seen a problem. Might be because of missing knowledge 
> about what the driver does at all or whatever. I don't know.
> 
Evaluation boards come with unlocked/unburned fuse so if a designer
wants to evaluate his fuse configuration on an EVB during the design
phase of his custom product he may need this driver especially when
using Freescale's MFG Tools.
If your Wandboard has unburned fuses and their kernel has this driver,
then ask them why they keep this features open to their users by default
(this can be because the SOM are intended to be used in final products
and they let the final user of their SOM burn what he wants in the fuse
 - his own MAC address for example - so here again that's the job of the
end product designer to remove this feature once he launch his product
on the market).

Best regards,
Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Bob Cochran]

On 11/09/2014 10:09 AM, Eric Bénard wrote:
> Hi Alexander,
>
> Le Sun, 09 Nov 2014 11:14:39 +0100,
> Alexander Holler <holler@ahsoftware.de> a écrit :
>
>> Am 08.11.2014 19:49, schrieb Alexander Holler:
>>
>>> I'm not confused, at least not in regard what you want to suggest. Of
>>> course, I'm totally confused about the fact that almost nobody else
>>> before has critized that write functionality of this driver, also I'm
>>> already used to the fact that I'm unable to understand many things which
>>> are happing in todays world. But nobody is perfect. ;)
>>
>> Just to make it more clear what this thread is about, here is a relevant
>> sentence copied form the reference manual for the chip:
>>
>> "In order to avoid "rogue" code performing erroneous writes to OTP, a
>> special unlocking sequence is required for writes to the fuse banks."
>>
> That's why adding an unlock sysfs entry would match the required
> sequence to unlock the write access on the user point of view, but
> that's Freescale's problem and policy.


Hi,

My preference would be to have an otp recipe under 
meta-fsl-arm/recipes-manufacturing and in the recipe would be the otp 
kernel patch that could be applied along with user space code to 
exercise it.

As an un-applied patch, I would never have to worry about it bricking my 
board.  However, when it comes time to use the feature in a 
manufacturing environment, it's there for me.

And for extra safety, give the patch an unusual suffix (e.g., .otp) so 
it doesn't accidentally get swept into a kernel patch set.

Bob






>
>> Now guess why the HW was designed this way. And then look again at the
>> driver which nullifies the careful implementation in the HW. It doesn't
>> have to be the fault of the author, e.g. he just might have written it
>> for internal use. The problem is that it went into kernels for public
>> use and nobody has seen a problem. Might be because of missing knowledge
>> about what the driver does at all or whatever. I don't know.
>>
> Evaluation boards come with unlocked/unburned fuse so if a designer
> wants to evaluate his fuse configuration on an EVB during the design
> phase of his custom product he may need this driver especially when
> using Freescale's MFG Tools.
> If your Wandboard has unburned fuses and their kernel has this driver,
> then ask them why they keep this features open to their users by default
> (this can be because the SOM are intended to be used in final products
> and they let the final user of their SOM burn what he wants in the fuse
>   - his own MAC address for example - so here again that's the job of the
> end product designer to remove this feature once he launch his product
> on the market).
>
> Best regards,
> Eric
>

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Nikolay Dimitrov]

Hi Alexander,

The eFuses are lockable. If you don't intend to further modify their
value after proper programming, you should lock them. If you don't do
so, you shouldn't blame the Linux driver for the consequences.

Crippling the IMX OTP driver doesn't solve the system security issues.
I can write to the IMX physical memory (e.g. to program OCOTP
registers) without this Linux driver at all, I just need proper
privileges and the devregs tool (thanks Eric & Troy!).

I have just like you some imx6 hobby boards. If one of them is bricked,
it's totally my fault. And yet we have hundreds of thousands of imx6
boards on the road, which we locked during manufacturing, and disabled
the OTP driver, so it's impossible to brick them via OTP.

To me it seems that the current state of affairs is already perfectly
OK and there's no need to panic.

Kind regards,
Nikolay

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Alexander Holler]

Am 09.11.2014 13:34, schrieb Nikolay Dimitrov:
> Hi Alexander,
>
> The eFuses are lockable. If you don't intend to further modify their
> value after proper programming, you should lock them. If you don't do
> so, you shouldn't blame the Linux driver for the consequences.
>
> Crippling the IMX OTP driver doesn't solve the system security issues.
> I can write to the IMX physical memory (e.g. to program OCOTP
> registers) without this Linux driver at all, I just need proper
> privileges and the devregs tool (thanks Eric & Troy!).
>
> I have just like you some imx6 hobby boards. If one of them is bricked,
> it's totally my fault. And yet we have hundreds of thousands of imx6
> boards on the road, which we locked during manufacturing, and disabled
> the OTP driver, so it's impossible to brick them via OTP.

First let me quote again the sentence from the reference manual you 
missed to quote:

"In order to avoid "rogue" code performing erroneous writes to OTP, a 
special unlocking sequence is required for writes to the fuse banks."


To conclude, you are saying:

- The (easy) write functionalty of the driver should be kept because
   it's of no use on locked devices anyway.

- Unlocked devices ("hobby boards", whatever that is) are second class
   devices where it's the owners fault if some malware, wrong script,
   typo or similiar writes to some file in the sysfs.

- The way to write to the OTP offered by the driver is the same as the
   way offered by the HW, which means the more complicated way
   implemented in the HW (which, by the way, is still in effect when
   using devregs) is senseless.


...


 > To me it seems that the current state of affairs is already perfectly
 > OK and there's no need to panic.

So revealing and trying to fix bugs nowadays means panicking?

I thought it's still part of the normal software development process.
Maybe I really have become oldschool and better get a towel.



Anyway, as I'm running out of hope and polite arguments (at least in
regard to this topic) and getting near to the point of using sentences
Linus has unfortunately become famous for, I better stop to comment
further.

Alexander Holler

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Nikolay Dimitrov]

Hi Alexander,

I really appreciate your efforts to explain your motivation for the
patch, and also respect your position, even if we didn't reached
consensus.

Thanks to everyone who contributed valuable thoughts on subject. I feel
that can't contribute anything else to the topic, so please excuse me.

Kind regards,
Nikolay

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Eric Bénard]

Hi Alexander,

Le Fri, 07 Nov 2014 16:23:10 +0100,
Alexander Holler <holler@ahsoftware.de> a écrit :

> Am 07.11.2014 16:07, schrieb Otavio Salvador:
> > On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
> >> Hi Jon,
> >>
> >> Le Fri, 7 Nov 2014 15:31:15 +0100,
> >> Jon Nettleton <jon.nettleton@gmail.com> a écrit :
> >>> Fair enough.  Can we all agree that it should default to disabled?
> >>
> >> On your product sure, on the evaluation board that's more questionable.
> >>
> >> When an evaluation board's user reach this sysfs entries to do a write
> >> to it I believe that's not a random choice but a user's choice.
> >>
> >> Maybe adding a otp_unlock entry to the sysfs before allowing write could
> >> secure more the feature when running on the evaluation boards so that
> >> the user can think twice before writing to the fuses.
> >
> > I like Eric's idea of the unlock sysfs entry for this purpose.
> 
> Maybe I should mention that this extremly dangerous driver already was 
> posted for submission into the (production) kernel and nobody had a 
> problem with it's write functionality, just with some other stuff.
> 
In LAKML there was a post for a driver (for 23/28 at the moment) where
write support was dropped :
http://comments.gmane.org/gmane.linux.drivers.devicetree/94343

In mainline kernel :
- on the i.MX there is only one function used to get the MAC :
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mxs/mach-mxs.c#n90
- read only fuse infrastructure was integrated for Tegra :
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/tegra/fuse

In Freescale's released kernel, the feature is available but one more
time I think that's not a production kernel but a base source code to
evaluate SOC's features on an evaluation board and then design
your own products where it's your responsibility to add / remove
features.

> And even if it's ok for FSL to promote this feature with their BSP, I 
> have absolutely no love for the way it was done. It now is likely part 
> of many production kernels, something never should have happened.
> 
It's the developer's responsibility to add/remove features on _their_
production kernel for _their_ product.

> And I still see absolutely no need to have some one time functionality 
> as part of the kernel, regardless how usefull it might be for some 
> developers. It's a waste of time and resources to include that write 
> functionality in something meant for the greater public. Especially 
> something that dangerous.
> 
If you look how Freescale has designed their "Manufacturing tool" and
check a few of their xml files, you will understand why they need this
kind of low level access features in the kernel ;-)

Here on real products we do all that at bootloader level using a
special production version of the bootloader but that's an other story.

Best regards,
Eric

Re: [PATCH 0/1] arm: imx: fsl_otp: make fuses (OTP memory) read-only

[Eric Bénard]

Hi Otavio,

Le Fri, 7 Nov 2014 13:07:26 -0200,
Otavio Salvador <otavio@ossystems.com.br> a écrit :
> On Fri, Nov 7, 2014 at 1:04 PM, Eric Bénard <eric@eukrea.com> wrote:
> > Maybe adding a otp_unlock entry to the sysfs before allowing write could
> > secure more the feature when running on the evaluation boards so that
> > the user can think twice before writing to the fuses.
> 
> I like Eric's idea of the unlock sysfs entry for this purpose.
> 
FWIW that's how it's done in barebox :
http://git.pengutronix.de/?p=barebox.git;a=blob;f=arch/arm/mach-imx/ocotp.c;h=837500fff8865d64ea2ed0f129cb2a8869e0aa7d;hb=HEAD#l430

http://git.pengutronix.de/?p=barebox.git;a=blob;f=arch/arm/mach-imx/ocotp.c;h=837500fff8865d64ea2ed0f129cb2a8869e0aa7d;hb=HEAD#l294

Eric