RfidResearchGroup / proxmark3

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

Get rid of BigBuf_get_addr() #899

Open slurdge opened 4 years ago

slurdge commented 4 years ago

This is blocking doegox/proxmark-internal#247. Right now, various functions are using the "address" of BigBuf as a magic variable to pass around data. Clearly this is prone to error and cannot be used with a proper allocator. So we should remove all calls to the BigBuf_get_addr() and replace them with a BigBuf_malloc() or equivalent function. Especially, we need to find what is the required space by those functions which often declare "I'll take as much as I need and pray that I don't overflow"

iceman1001 commented 4 years ago

O boy, this actually means documenting how stuff works on the device side :)

slurdge commented 4 years ago

This is related to doegox/proxmark-internal#243. I'm quite sure if we touch it something will break.

iceman1001 commented 4 years ago

... yeah..

putting a blank t55x7 card on top of the antenna and run lf search will trigger much. I adjusted the stack limit to 9K, it doesn't warn any longer but now clone, read that t55x7 card doesn't work...

a can of worms.

slurdge commented 4 years ago

Yeah, my guesstimate was the stack before was around 12K because we increased bigbuf from 40->48K. However the commands still should work because we didn't reorder much. I started looking at the usage of get_addr(), it ain't pretty...

iceman1001 commented 4 years ago

Not that many places left for BigBuf_get_addr() calls. This below is from the FPGA branch, but more and more have been converted into bb_malloc style.

armsrc/start.c
26:    next_free_memory = BigBuf_get_addr();

armsrc/pcf7931.c
19:    uint8_t *dest = BigBuf_get_addr();

armsrc/lfsampling.c
137:            data.buffer = BigBuf_get_addr();
149:        data.buffer = BigBuf_get_addr();
404:    uint8_t *dest = BigBuf_get_addr();
490:    uint8_t *dest = BigBuf_get_addr();
552:    uint8_t *dest = BigBuf_get_addr();

armsrc/lfops.c
505:    signed char *dest = (signed char *)BigBuf_get_addr();
655:    uint32_t *buf = (uint32_t *)BigBuf_get_addr();
709:    char *dest = (char *)BigBuf_get_addr();
812:    uint8_t *buf = BigBuf_get_addr();
901:    uint8_t *dest = BigBuf_get_addr();
1019:    uint8_t *dest = BigBuf_get_addr();
1032:    uint8_t *dest = BigBuf_get_addr();
1045:    uint8_t *dest = BigBuf_get_addr();
1056:    uint8_t *dest = BigBuf_get_addr();
1062:    uint8_t *dest = BigBuf_get_addr();
1132:    uint8_t *dest = BigBuf_get_addr();
1179:    uint8_t *dest = BigBuf_get_addr();
1235:    uint8_t *dest = BigBuf_get_addr();
1345:    uint8_t *dest = BigBuf_get_addr();
1450:    uint8_t *dest = BigBuf_get_addr();
1525:    uint8_t *dest = BigBuf_get_addr();
2061:    uint8_t *buf = BigBuf_get_addr();

armsrc/i2c.c
789:    uint8_t *fwdata = BigBuf_get_addr();

armsrc/felica.c
752:    uint8_t *dest = BigBuf_get_addr();

armsrc/appmain.c
324:    uint8_t *test_data = BigBuf_get_addr();
1508:            uint8_t *mem = BigBuf_get_addr();
1689:            uint8_t *mem = BigBuf_get_addr();
1743:            uint8_t *mem = BigBuf_get_addr();

armsrc/Standalone/lf_icehid.c
81:    uint8_t *dest = BigBuf_get_addr();
133:    uint8_t *dest = BigBuf_get_addr();
193:    uint8_t *dest = BigBuf_get_addr();
237:    uint8_t *dest = BigBuf_get_addr();

armsrc/Standalone/lf_em4100rwc.c
141:    bba = BigBuf_get_addr();

armsrc/Standalone/lf_em4100rswb.c
314:    bba = BigBuf_get_addr();

armsrc/Standalone/lf_em4100emul.c
88:    bba = BigBuf_get_addr();

armsrc/Standalone/hf_14asniff.c
100:        uint8_t *trace_buffer = BigBuf_get_addr();

armsrc/BigBuf.c
170:    uint8_t *trace = BigBuf_get_addr();
slurdge commented 4 years ago

That's awesome! I'm not too worried about the lf_*.c stuff, from a quick glance, this is more of the same, so replacing one should mean replacing all pretty easily. The big one IMHO is the sneaky last one: it's related to trace, and I suspect all trace function silently assume this... However if there is only that one left, then maybe we can do the switch to the new allocator at the same time we are removing it ...

iceman1001 commented 4 years ago

to be honest, I think this bigbuf / stack connected issues was the reason for my problems with Hitag2 stuff.
I think bringing this to the surface gave us a chance to solve it properly once and for all.