search

RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
4.08k stars 1.07k forks source link

Increased stack size conflicts with BT addon #806

Closed aczid closed 4 years ago

aczid commented 4 years ago

Describe the bug When the bluetooth module is enabled the proxmark does not behave as expected.

To Reproduce Steps to reproduce the behavior:

  1. Enable PLATFORM_EXTRAS=BTADDON in Makefile.platform in latest master
  2. make clean && make && ./pm3-flash-all
  3. ./client/proxmark3 hw tune`
  4. The test shows the wrong results for LF, LF commands don't work, everything seems buggy
  5. If you change the Makefile.platform file back or change the stack size in common_arm/ldscript.common back to 6K everything works as normal again.

Expected behavior I would like hw tune and other LF commands to work as normal again and also be able to use the BT feature.

Desktop:

[+] HF antenna: 46.68 V - 13.56 MHz [+] HF antenna is OK

Fl0-0 commented 4 years ago

Same issue with :

PLATFORM=PM3RDV4
PLATFORM_EXTRAS=BTADDON FPC_USART_DEV
STANDALONE=HF_LEGIC

Fixed by reverting a53c4a8a5bbc2c034d3f4341a5821f222b0d0e2e

doegox commented 4 years ago

cc @slurdge & @iceman1001

slurdge commented 4 years ago

Yeah, probably we are seeing now what was invisible before: there are different stack size for different scenarios. However what's strange in this case is that BigBuf should have overwritten part of the stack, but I guess we were lucky in that regard.

doegox commented 4 years ago

What about declaring stack size such as BigBuffer is at least the same size as before (39488 -> 40000)? This means a stacksize of max 8488. @aczid can you test with stacksize = 8488 please ?

iceman1001 commented 4 years ago

As @slurdge mentioned, this opened up a can of worms. With stuff is brought up to surface. Which is good, it means we can fix it once and for all. It also sucks majorly.

iceman1001 commented 4 years ago

I was going through the bigbuf_get_address calls to make sure those works nice. However LF sampling and decoding uses the same. ie, save sample data from index0, then decode and store decoded data from index0 (overwritting the samples)... then to make it really nice, we can add the logtrace... which also uses index0...

iceman1001 commented 4 years ago

BT enabled w addon.

image

image

HF ok

hf search
hf sniff 1000
hf 14a sniff c r

LF fails on a blank T55x7

lf search - >  stackoverflow

LF on a programmed T55x7 works

[usb] pm3 --> lf t55 det
[=]      Chip Type      : T55x7
[=]      Modulation     : ASK
[=]      Bit Rate       : 2 - RF/32
[=]      Inverted       : No
[=]      Offset         : 33
[=]      Seq. Term.     : Yes
[=]      Block0         : 0x000880E0
[=]      Downlink Mode  : default/fixed bit length
[=]      Password Set   : No

[usb] pm3 --> lf t55 du
[+] Reading Page 0:
[+] blk | hex data | binary                           | ascii
[+] ----+----------+----------------------------------+-------
[+]  00 | 000880E0 | 00000000000010001000000011100000 | ....
[+]  01 | 00000000 | 00000000000000000000000000000000 | ....
[+]  02 | 00000000 | 00000000000000000000000000000000 | ....
[+]  03 | 00000000 | 00000000000000000000000000000000 | ....
[+]  04 | 00000000 | 00000000000000000000000000000000 | ....
[+]  05 | 00000000 | 00000000000000000000000000000000 | ....
[+]  06 | 00000000 | 00000000000000000000000000000000 | ....
[+]  07 | 00000000 | 00000000000000000000000000000000 | ....
[+] Reading Page 1:
[+] blk | hex data | binary                           | ascii
[+] ----+----------+----------------------------------+-------
[+]  00 | 000880E0 | 00000000000010001000000011100000 | ....
[+]  01 | E0150A8C | 11100000000101010000101010001100 | ....
[+]  02 | 55462D86 | 01010101010001100010110110000110 | UF-.
[+]  03 | 00000000 | 00000000000000000000000000000000 | ....
[+] saved to json file lf-t55xx-dump.json
[+] saved 12 blocks to text file lf-t55xx-dump.eml
[+] saved 48 bytes to binary file lf-t55xx-dump.bin

[usb] pm3 --> lf visa clone 112233
[=] Preparing to clone Visa2000 to T55x7 with CardId: 112233
[+] Blk | Data
[+] ----+------------
[+]  00 | 00148068
[+]  01 | 56495332
[+]  02 | 0001B669
[+]  03 | 00000183
[+] Success writing to tag
[+] Done
[usb] pm3 --> lf t55 det
[=]      Chip Type      : T55x7
[=]      Modulation     : ASK
[=]      Bit Rate       : 5 - RF/64
[=]      Inverted       : No
[=]      Offset         : 32
[=]      Seq. Term.     : Yes
[=]      Block0         : 0x00148068
[=]      Downlink Mode  : default/fixed bit length
[=]      Password Set   : No

[usb] pm3 --> lf visa read
[+] Visa2000 Tag Found: Card ID 112233,  Raw: 564953320001B66900000183

[usb] pm3 --> lf search
[=] NOTE: some demods output possible binary
[=] if it finds something that looks like a tag
[=] False Positives ARE possible
[=]
[=] Checking for known tags...
[=]
[+] Visa2000 Tag Found: Card ID 112233,  Raw: 564953320001B66900000183

[+] Valid Visa2000 ID found!

[+] Chipset detection: T55xx
[usb] pm3 -->
Fl0-0 commented 4 years ago

Works fine with 8488: at least basic LF / HF operations and the standalone mode.

diff --git a/common_arm/ldscript.common b/common_arm/ldscript.common
index 443d9825..610e5c0b 100644
--- a/common_arm/ldscript.common
+++ b/common_arm/ldscript.common
@@ -9,7 +9,7 @@ ms of the GNU GPL, version 2 or,
 -----------------------------------------------------------------------------
 */

-stacksize = DEFINED(stacksize) ? stacksize : 9K;
+stacksize = DEFINED(stacksize) ? stacksize : 8488;
 commonareasize = 0x20;

 /* AT91SAM7S256 has 256k Flash and 64k RAM */
[#] Various
[#]   Max stack usage.........4096 / 8480 bytes
[#]   DBGLEVEL................1
[#]   ToSendMax...............-1
[#]   ToSendBit...............0
[#]   ToSend BUFFERSIZE.......2308
[#]   Slow clock..............31503 Hz
doegox commented 4 years ago

Fixed by https://github.com/RfidResearchGroup/proxmark3/commit/b5345eb0bd83d53a0f4241ce6c726c238dec32a1 for now

slurdge commented 4 years ago

Yep. The thing is before, we could have this situation: [Bigbuf....][StackEnd StackStart], where stack was growing but by change bigbuf was not used at that time. If lucky, everything goes well. If a little bit unlucky, the values in bigbuf are the values in stack, since there was absolutely no check done. If very unlucky, the bigbuf would overwrite the stack and corrupt values here.

The thing now we can see if the stack is too short. However we can't see if the bigbuf is too short, work is underway for this. Once we know how much bigbuf & stack is needed for each workflow, we will be able to either:

  1. build different versions with the Makefile (BTW you can define the stack size in the makefile with linker params, no need to change ldscript);
  2. dynamically adjust (at runtime) the frontier, however this is a difficult exercise
doegox commented 4 years ago

If I trust the tests done so far, BigBuff needs to be 40k and stack needs to be something > 6k, so right now, the cursor can't be moved much compared to what it was before. But as you said at least we start getting visibility and large allocations can be challenged

iceman1001 commented 4 years ago

Slowly, going throu the LFOPS.c... refactoring as I go.
It sadly becomes both NG refactoring, renaming, and dynamic alloc adaptations with checking for max available bigbuff space. A bunch of watch commands now behavies nice again. 42xx stack. Next step, t55x7 commands where I think much problems is.

aczid commented 4 years ago

Fixed by b5345eb for now

I confirm this issue is fixed. Thank you!

iceman1001 commented 4 years ago

This confirms, its inside DoAcquisition()

image

iceman1001 commented 4 years ago

Actually no. When you have a "wiped" clean t5577 on the antenna and run lf search
its inside the hitag2 getuid that breaks it.

I knew I had issues with HItag2 but this was a good thing to find.

image