search

Proxmark / proxmark3

Proxmark 3
http://www.proxmark.org/
GNU General Public License v2.0
3.11k stars 903 forks source link

Bug: Failing to get correct values from device -> client #544

Closed iceman1001 closed 6 years ago

iceman1001 commented 6 years ago

Bug: Failing to get correct values from device -> client. It has todo with that Cmd, arg0,arg1,arg2 is defined as uint32_t in the function cmdsend, (from device -> client) as seen here: client: https://github.com/Proxmark/proxmark3/blob/master/common/cmd.h#L41 device: https://github.com/Proxmark/proxmark3/blob/master/armsrc/apps.h#L190

problem, is that usbcommand is defined uint64_t as seen here. https://github.com/Proxmark/proxmark3/blob/3851172d8168f32f1dc28b48b7a4df88e30bdeca/include/usb_cmd.h#L30

I added a fix for it in iceman fork.

pwpiwi commented 6 years ago

This isn't a bug. The compiler implicitely casts the uint32_t to uint64_t. There is not much to send from device to client (CMD_ACK usually).

iceman1001 commented 6 years ago

No, I disagree with the implicite cast, since I got not my values back from the device when it was uin32_t on mingw dev env. Your second argument is just unvalid.

pwpiwi commented 6 years ago

Can you please give an example where the uint32_t is not sufficient? Where is an issue with truncated values when sending from device to client?

iceman1001 commented 6 years ago

The issue is when I want a uint64_t sent and I get a trunctated uin32_t back, when the structure used says it will return a uint64_t then there is a issue. For the hw tune the measurments doesn't fit into its assigned 16b slot. We could compact it into 18bits in order to fit maximum measurements for 130000. Two of those values doens't fit into a uint32. However usbcommand struct says uint64 for the arg[], which it should fit with ease.

possible solutions;

  1. accept faulty code and put values in the data[512] instead.
  2. fix the faulty functions parameters.

I opted for solution 2 in iceman fork. Which ever solution pm3 offical wants, I'm fine with it.

pwpiwi commented 6 years ago

That would only be an issue for voltages above 65535mV. There are reasonable doubts if voltages above 47000mV are possible.

iceman1001 commented 6 years ago

If you look at current devices yes,

pwpiwi commented 6 years ago

OMG, we now even have to support hardware which nobody has seen before... :smile:

Raised PR #546 to scale LF voltages

iceman1001 commented 6 years ago

Interesting, a man of science calling out the name of god in order to make argument better. I see the bug is still in the your PR rendering it useless.

pwpiwi commented 6 years ago

The PR is meant to allow voltages over 65535mV. It is not meant to fix what is a bug in your opinion and what is not a bug in my opinion.

proxmark3> hw tune

Measuring antenna characteristics, please wait.........
# LF antenna: 82.09 V @   125.00 kHz
# LF antenna: 84.97 V @   134.00 kHz
# LF optimal: 90.20 V @   126.32 kHz
# HF antenna:  0.64 V @    13.56 MHz
# Your HF antenna is unusable.
Displaying LF tuning graph. Divisor 89 is 134khz, 95 is 125khz.

proxmark3>

image