feat: load firmware over SysEX - #295
Conversation
|
Could this be used as an attack vector? At one point, you needed to run from a https:// in order to get to SysEx from a web browser. Not sure if that rule is still in effect. |
|
yes, it is going to need a community feature setting which is off by default. The browser is supposed to ask the user for giving the client permission to send sysex, but there are other access points so it's safest to opt-in from the deluge side as well. |
|
Having a Community Feature setting sounds like a good way to cover this. This can speed-up development too. It takes 40 or more seconds to transfer an image via JLink. |
|
You can't imagine how much time I invest in mounting, copying, unmounting, inserting card, .... This is going to be a huge improvement for development |
|
Ahhh this is going to be amazing for building ... so much time invested in removing / inserting my SD cards ... my hand was feeling pain at one point lol. |
|
Thanks, gonna start working on deluge ransomware shortly Although in practice at least in firefox it's https only and permissions are per site (and a seperate permission for sysex), so it'd depend on how much you trust people not to grant access to random websites |
259ae00 to
32541fa
Compare
|
Added a community setting feature, RCE is now disabled by default. There is also a random 31-bit identifier generated when you enable it (persistent across reboots), which you have to supply when loading from a computer. It is nowhere near cryptographically secure (it is not like the full MIDI/driver stack has been audited/fuzzed for untrusted data anyway..), but at least you cannot just send a single non-specific and obvious MIDI message to get an unknown (but possibly dev-enabled) deluge to execute some code. It also helps to avoid mistakes (it is a way to say "trust me, I am a dev, I know what I am doing") |
|
How do you supply the 31-bit identifier? I have the app SySex Librarian in mac that allows you to load a .syx message and transfer it to any USB midi device connected. How is it going to work for the .bin files? is there going to be a modified build process that also outputs a .syx file? |
7c267e8 to
0bd8e8e
Compare
|
@soymonitus so far there is a source code program included as Need to figure out the best way to integrate client code in the build system as we yet don't assume any host c compiler. Perhaps binaries could be included as part of the binary toolchain. |
2b11203 to
763bbb1
Compare
sapphire-arches
left a comment
sapphire-arches
left a comment
There was a problem hiding this comment.
Really looking forward to having this, major quality of life improvement possible. Some minor comments but mostly LGTM
| disableTimer(TIMER_SYSTEM_FAST); | ||
| disableTimer(TIMER_SYSTEM_SUPERFAST); | ||
|
|
||
| auto ptr = (void (*)(uint32_t, uint32_t, uint32_t, char*))funcbuf; |
There was a problem hiding this comment.
Strictly speaking we should probably issue a IMB (instruction memory barrier) prior to jumping off in to the great beyond here. That would avoid the need for writing through the UNCACHED_MIRROR_OFFSET above I think.
There was a problem hiding this comment.
I've spent quite a long time going through all ARM guides about self-modifiying code and tried all the standard options which "are supposed to" work. The renesas implementation of the arm cache hierarchy (and the co-processors as documented in the armv7a manual) simply does not follow the ARM documentation and none of the standard rules apply. UNCACHED_MIRROR_OFFSET as suggested by rohan does work though, which is why the code uses it.
There was a problem hiding this comment.
argh, thats annoying but not really as surprising as I would like it to be. perhaps manually poking some of the cache management registers would work but we can ship this for now.
| } | ||
|
|
||
| // Return the CRC of the bytes buf[0..len-1]. | ||
| uint32_t get_crc(uint8_t* buf, int len) { |
There was a problem hiding this comment.
FatFS and sd_util.c already have CRC implementations, can we reuse one of those?
There was a problem hiding this comment.
maybe, but the idea is that this file pack.c is self-contained as it is meant to be used both by the deluge and client software. Also I could remember wrong, but those used the slow naive method instead of the table method so I think those could reuse this rather than the other way around.
There was a problem hiding this comment.
No problem, it's quite small.
| return; // Do not do that | ||
| } | ||
| // data[4]: reserved, could serve as a message identifier to filter messages per category | ||
| uint8_t reply_hdr[5] = {0xf0, 0x7d, 0x03, 0x40, 0x00}; |
There was a problem hiding this comment.
I think we can use the shiny new official sysex ID here, no? (00H 21H 7BH)
There was a problem hiding this comment.
yes, but that would be a separate PR which modifies everything. My idea was to let the unofficial prefix namespace grow "organically" (ie into a big mess) as we add functionality for some more time. And later do a restructure of the messages at the same time we switch to the new prefix.
There was a problem hiding this comment.
Sounds good, thanks.
| return; // Do not do that | ||
| } | ||
| // data[4]: reserved, could serve as a message identifier to filter messages per category | ||
| uint8_t reply_hdr[5] = {0xf0, 0x7d, 0x03, 0x40, 0x00}; |
There was a problem hiding this comment.
Sounds good, thanks.
| disableTimer(TIMER_SYSTEM_FAST); | ||
| disableTimer(TIMER_SYSTEM_SUPERFAST); | ||
|
|
||
| auto ptr = (void (*)(uint32_t, uint32_t, uint32_t, char*))funcbuf; |
There was a problem hiding this comment.
argh, thats annoying but not really as surprising as I would like it to be. perhaps manually poking some of the cache management registers would work but we can ship this for now.
| } | ||
|
|
||
| // Return the CRC of the bytes buf[0..len-1]. | ||
| uint32_t get_crc(uint8_t* buf, int len) { |
There was a problem hiding this comment.
No problem, it's quite small.
|
I suggest we leave this one out of the first community release. We can put it in when we have some facility on the user-side (like a web application) that makes use of it. A "feature flag" could handle this. |
|
I was thinking a native utility as well which could be made an (optional) step in the build system. Currently I do
you mean like a preprocessor macro which is not defined by default but can be supplied on the cmdline, like is done in the wren PR? I could do that. |
|
A Wren-like preprocessor macro sounds good. |
|
I would like to see a Javascript library for calling SysEx. It may already exist. Then we could even have a "flash from the web" facility. |
Is there a reason you don't want to include this? I would like to have it in the first release so it is "connectivity ready" as some vendors call it. Once we get the webpage to a point where it allows easy update with firmware selection I'd like the community deluges to be ready for that and not wait until the next release that people need to install manually again |
|
I hope to get the javascript stuff in order and upstream it into this repo quite soon. Although next week might be more realistic than this week. |
|
Added a compile time flag |
2063712 to
045dfe2
Compare
|
I have another reason to put this in the firmware. The bootloaders up to version 5 can break with 0 byte firmware images. If we allow flashing via Webpage we can prevent this from happening after the first successfull flash |
|
I also have a prototype which allows you to select a .bin file from the sdcard in a menu from the flashed firmware. If there is no outstanding issues with this PR, I would like to see this one merged first (for my own usecase it doesn't matter if it is enabled in binaries or not), and I will submit that as a follow-up. |
|
Is the process of updating firmware over sysex explained in the docs somewhere? |
7 participants
Need to clean up this mess but can somewhat reliably load a new firmware image over the existing one over USB in ~3.5 seconds.