You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/08/09 02:50:45 UTC
[GitHub] [incubator-nuttx] rm5248 opened a new pull request, #6815: pic32mx: add option to disable JTAG at runtime
rm5248 opened a new pull request, #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815
## Summary
Add a new option to optionally disable JTAG at runtime. Defaults to enabling
JTAG at runtime as that is the state the processor comes up in. This only
affects 3xx/4xx/5xx/6xx/7xx series of processors as the 1xx/2xx series of
processors have a different method of configuring JTAG.
## Impact
By default, JTAG will remain enabled on the processors. This is the default
that the processor comes up in.
## Testing
Validated with my explorer 16 board and PIC32MX. I made sure that both settings
of the JTAG option work correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941174731
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
What is the HW default?
Why not do this as compile time option?.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] rm5248 commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r943051992
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
I've configured it to disable JTAG by default. Conditionally enabling the functions in terms of code space doesn't make sense(as any level of optimization will cause uncalled methods to not emit any code), but it might make sense to conditionally compile functions in or out to suppress compiler warnings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941296912
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
I'm a bit confused -- I thought this was a compile-time option (with the new Kconfig)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#issuecomment-1211879044
@rm5248
I guess the issue is an unfortunate consequence of pin choices and reuse on the board you have.
>I'm therefore extremely confused as to what exactly you think this should be.
I am sorry that is my fault, I did not read this well enough and I assumed MC was consistent in config and security.
Without fuse or bootloader Device Configuration Data to disable JTAG how do they expect the device to be secured?
Maybe they punted on Fuses and the code to do this sort of thing supposed to be placed in the 12 KB boot Flash?
The best thing to do put it there in the boot flash. The next best thing is to add code in the __start as soon as there is enough clock initialization done to modify the DDPCON and set it to the Kconfig. If the soonest this can be don is a in C code a 1 line `modifyreg32` can be used and the set/clears select by the Kconfig.
In any case the default of the Kconfig should be JTAG disabled.
There is still a security hole if the code is in __start:, If on boot JTAG is enabled a hack would be to set the IP to skip the disable code. I would think we are missing something here, because MC would not leave this open....
```
Note the following details of the code protection feature on Microchip devices:
• Microchip products meet the specification contained in their particular Microchip Data Sheet.
• Microchip believes that its family of products is one of the most secure families of its kind on the market today, when used in the intended manner and under normal conditions.
• There are dishonest and possibly illegal methods used to breach the code protection feature. All of these methods, to our
knowledge, require using the Microchip products in a manner outside the operating specifications contained in Microchip’s Data Sheets. Most likely, the person doing so is engaged in theft of intellectual property.
• Microchip is willing to work with the customer who is concerned about the integrity of their code.
• Neither Microchip nor any other semiconductor manufacturer can guarantee the security of their code. Code protection does not
mean that we are guaranteeing the product as “unbreakable.”
Code protection is constantly evolving. We at Microchip are committed to continuously improving the code protection features of our
products. Attempts to break Microchip’s code protection feature may be a violation of the Digital Millennium Copyright Act. If such acts
allow unauthorized access to your software or other copyrighted work, you may have a right to sue for relief under that Act.```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#issuecomment-1211838516
@rm5248 - Do you have a link to the version of the reference manual you are using?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r940853842
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -155,6 +156,60 @@ static inline void pic32mx_cache(void)
asm("\tmtc0 %0,$16,0\n" : : "r" (regval));
}
+/****************************************************************************
+ * Name: pic32mx_disable_jtag
+ *
+ * Description:
+ * Disable the JTAG connection
+ *
+ * Assumptions:
+ * Interrupts are disabled.
+ *
+ ****************************************************************************/
+
+static inline void pic32mx_disable_jtag(void)
Review Comment:
Should new interfaces be wrapped with ifdef to avoid compilation warning about unused code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#issuecomment-1212061426
Since everything was clear now, I merged it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] rm5248 commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941911375
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -155,6 +156,60 @@ static inline void pic32mx_cache(void)
asm("\tmtc0 %0,$16,0\n" : : "r" (regval));
}
+/****************************************************************************
+ * Name: pic32mx_disable_jtag
+ *
+ * Description:
+ * Disable the JTAG connection
+ *
+ * Assumptions:
+ * Interrupts are disabled.
+ *
+ ****************************************************************************/
+
+static inline void pic32mx_disable_jtag(void)
Review Comment:
I wasn't sure if these should be public functions at all, so I simply made them static for simplicity. From the datasheet it seems to indicate that you can turn JTAG on and off at runtime(although exactly why you would do that is a mystery to me).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r942835457
##########
arch/mips/src/pic32mx/Kconfig:
##########
@@ -1019,6 +1019,14 @@ config PIC32MX_GPIOIRQ
---help---
Build in support for interrupts based on GPIO inputs from IOPorts
+config PIC32MX_JTAG_ENABLE
+ bool "Enable JTAG"
+ default y
Review Comment:
@rm5248 I suggest you to change it to "default n" to avoid the security issues that David raised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r942304646
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
This may be considered a security issue. It is also a minor waste of code space. It would be better to have a compile time setting to Disable in the first code after reset, that is if it can not be disabled by a fuse setting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r942834326
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
@davids5 I understood your point, but I suggest just changing in the Kconfig to "default n". Notice that STM32 and other arches also defines JTAG_ENABLE and similar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] rm5248 commented on pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
rm5248 commented on PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#issuecomment-1211802399
@davids5 Using DEVCFG0 to disable JTAG is only available on the 1XX and 2XX series of PIC32MX, hence why I'm trying to disable JTAG on the 3XX/4XX/5XX/6XX/7XX series of chips. The datasheet and pic32mx_head.S agree with this, since what you linked to is the DEVCFG0 for the 1XX/2XX series - the DEVCFG0 for everything else doesn't have the `DEVCFG0_JTAGEN | \ ` line of code.
By default, JTAG is enabled on the PIC32MX unless it is explicitly disabled via DEVCFG0 on the 1XX and 2XX series, or via the DDPCON register at runtime on the 3XX/4XX/5XX/6XX/7XX series which is why I originally set this to enable JTAG by default(it should be a no-op).
The problem that I am seeing is that when JTAG is enabled, the JTAG pins can't be used for normal GPIO at a minimum(e.g. the LEDs connected to those pins do not turn on). I don't know if other chips use a peripheral pin select to change their settings(I am using a PIC32MX795F512L).
I'm therefore _extremely_ confused as to what exactly you think this should be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] rm5248 commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941913025
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
The PIC32MX defaults to enabling JTAG, so by default calling `pic32mx_enable_jtag` should be a no-op. I did it explicitly since otherwise it was more difficult to follow the logic(double negation). The option `PIC32MX_JTAG_ENABLE` was chosen since there is already a `PIC32MZ_JTAG_ENABLE`, but if there is a better name for it that would be good to know.
I'll fix those compiler errors in a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941424907
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
Please fix these compilation errors:
```
chip/pic32mx_lowinit.c: In function 'pic32mx_disable_jtag':
Error: chip/pic32mx_lowinit.c:172:21: error: unused variable 'regval' [-Werror=unused-variable]
register uint32_t regval;
^
chip/pic32mx_lowinit.c: In function 'pic32mx_enable_jtag':
Error: chip/pic32mx_lowinit.c:199:21: error: unused variable 'regval' [-Werror=unused-variable]
register uint32_t regval;
^
chip/pic32mx_lowinit.c: In function 'pic32mx_lowinit':
Error: chip/pic32mx_lowinit.c:250:5: error: "CONFIG_PIC32MX_JTAG_ENABLE" is not defined [-Werror=undef]
#if CONFIG_PIC32MX_JTAG_ENABLE
^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:82: pic32mx_lowinit.o] Error 1
make[1]: Target 'libarch.a' not remade because of errors.
make: *** [tools/LibTargets.mk:152: arch/mips/src/libarch.a] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 290: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#issuecomment-1211639776
@rm5248 Please have a look at https://github.com/apache/incubator-nuttx/blob/2335b69120a3e36f1aa0cecc19c95208806cae54/arch/mips/src/pic32mx/pic32mx_head.S#L638
and https://github.com/apache/incubator-nuttx/blob/c55085c0d8864c9401de97872ab3463d474f262c/arch/mips/src/pic32mx/Kconfig#L1109
```
DEBUG<1:0>: Background Debugger Enable bits (forced to ‘11’ if code-protect is enabled)
11 = Debugger disabled
10 = Debugger enabled
01 = Reserved (same as ‘11’ setting)
00 = Reserved (same as ‘11’ setting)
```
It would be the correct place to manage security in the boot sequences AND allow JTAG to be useful from boot. Anything else is a security risk and may allow debugging when it is needed
I would would strongly advise the PR be closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r941427112
##########
arch/mips/src/pic32mx/pic32mx_lowinit.c:
##########
@@ -192,6 +247,12 @@ void pic32mx_lowinit(void)
up_earlyserialinit();
#endif
+#if CONFIG_PIC32MX_JTAG_ENABLE
Review Comment:
> What is the HW default? Why not do this as compile time option?.
I'm also confuse, could you please explain @davids5 ? He created an Kconfig option to disable JTAG, same config exist for STM32, and others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a diff in pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on code in PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815#discussion_r942866595
##########
arch/mips/src/pic32mx/Kconfig:
##########
@@ -1019,6 +1019,14 @@ config PIC32MX_GPIOIRQ
---help---
Build in support for interrupts based on GPIO inputs from IOPorts
+config PIC32MX_JTAG_ENABLE
+ bool "Enable JTAG"
+ default y
Review Comment:
If you're concerned about security (I assume preventing unwanted code download capability?) then the Help for this Kconfig should document some kind of a warning to that effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #6815: pic32mx: add option to disable JTAG at runtime
Posted by GitBox <gi...@apache.org>.
acassis merged PR #6815:
URL: https://github.com/apache/incubator-nuttx/pull/6815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org