Fix proposal for hardfault with batt_smbus module #15789

flochir · 2020-09-22T09:45:31Z

Added buffer length check to SMBus::block_read (triggered hardfault in batt_smbus module by writing beyond buffer end due to device returning longer byte_count than requested/expected)

I ran into a hardfault when reading Smart Battery data with SMBus::block_read.

The fault is triggered in line 126 of
/Firmware/src/lib/drivers/smbus/SMBus.cpp
when memcpy writes beyond the end of the buffer “data” provided by the caller.

In my case the smbus device returned a byte count larger than the requested length (13 instead of 8 bytes).

I fixed this by adding a check for the length limit before the memcpy call, see

I experienced hardfaults working on a SMBUS battery driver based on batt_smbus. They werde triggered by the device returning a bigger byte_count value than was requested (13 bytes in byte_count versus a buffer length of 8 in my case), which lead to memcpy(data, &rx_data[4], byte_count); writing beyond the limit of the provided buffer.

As a fix I added a buffer length check to SMBus::block_read to force the maximum received data length to the length of the buffer provided by the caller.

I experienced that issue with a 3DR Solo battery among others.

…n batt_smbus module by writing beyond buffer end due to device returning longer byte_count than requested/expected)

dagar · 2020-09-22T12:37:13Z

Minor code styles changes.

src/lib/drivers/smbus/SMBus.cpp

Co-authored-by: Daniel Agar <daniel@agar.ca>

BazookaJoe1900 · 2020-09-22T18:44:20Z

@flochir Thanks for notice.
before I approve of the change, which seems correct. I have questions.
do you have idea who can it be that byte byte_count from the device (comes from rx_data[3]) doesn't match to the expected length?
have you changed the packets you read from the smart battery? is it possible that you read packet on the wrong expected size?

I have also added commit to fix compilation issue on some boards (airmind_mindpx-v2_default for example).
@dagar any idea why there is difference? shouldn't the calling for math lib be either exist or not on all compilations?

src/lib/drivers/smbus/SMBus.cpp

dagar · 2020-09-23T18:34:49Z

I have also added commit to fix compilation issue on some boards (airmind_mindpx-v2_default for example).
@dagar any idea why there is difference? shouldn't the calling for math lib be either exist or not on all compilations?

I'm not seeing the difference? What could be different is the linking dependencies, which often aren't 100% correct because you typically get common dependencies (like mathlib) included by something else.

jimdgit

But why did you leave cpy_len in?

BazookaJoe1900 · 2020-09-24T05:19:56Z

But why did you leave cpy_len in?

Can you explain? what is cpy_len? @jimdgit

jimdgit · 2020-09-24T13:29:39Z

The code was changed, so it is not longer an issue.
I also do not understand why you would prefer a call to a lib over the conditional operator ? :, which is almost free time wise compared to the lib call. In other contexts I would not question it, but in this one I do.

BazookaJoe1900 · 2020-09-24T16:15:25Z

I didn't got answer from @flochir yet how he get to this situation.
but on my side the pr is ready for merge, @dagar can you squash the commits?

dagar · 2020-09-24T16:20:32Z

The code was changed, so it is not longer an issue.
I also do not understand why you would prefer a call to a lib over the conditional operator ? :, which is almost free time wise compared to the lib call. In other contexts I would not question it, but in this one I do.

If you're referring to math::min those are all static constexpr, so it ends up being the same thing, but with a bit of type safety and easier to read. https://github.com/PX4/Firmware/blob/0dc8bb9c864dc49bbbe255ea0177480b46522d7d/src/lib/mathlib/math/Limits.hpp#L54-L57

jimdgit · 2020-09-24T16:31:59Z

I see ok great!

src/lib/drivers/smbus/SMBus.cpp

Co-authored-by: Beat Küng <beat-kueng@gmx.net>

bkueng

Thanks

flochir · 2020-09-25T17:13:26Z

@BazookaJoe1900
The way I understand SMBUS block read, the amount of bytes returned on a specific command is specified by the slave.
See http://smbus.org/specs/SMBus_3_0_20141220.pdf page 42.

So there really is no way to request a specific amount of bytes from the client. The slave will send the amount of bytes associated to that command. I2C::transfer() will limit its amount of read bytes to recv_len, but the byte_count originates directly from the device and will still be the full length of data available. To respect that on a lower level the SMBus driver may need to overwrite I2C::transfer().

Btw. there may be another issue to check. What happens if recv_len passed to I2C::transfer() is longer than the amount of data the SMBUS slave wants to transmit? Timeouts? Maybe there's some error or optimization potential here.

jimdgit · 2020-09-25T17:19:58Z

I 100% agree. We should not be messing with that.

What happens if recv_len passed to I2C::transfer() is longer than the amount of data the SMBUS slave wants to transmit?

That needs to be handled elsewhere and so far there is no evidence to that effect. We don't want to break other things.
Plus we have no test case for that.

flochir · 2020-09-25T17:58:46Z

That needs to be handled elsewhere and so far there is no evidence to that effect. We don't want to break other things.
Plus we have no test case for that.

It's directly subject to the SMBus protocol, so I don't see where else it could be handled. I'll see if I can run some tests against my setup.

BazookaJoe1900 · 2020-09-25T18:05:48Z

@flochir thanks for the notes.
Have you tried to read a packet which isn't on the master code? is it possible that you called it with wrong byte_count?
maybe you are working with something else then smart-battery?
I am working a lot with the SMBUS, and never got hardfault, so I wonder what is the difference

flochir · 2020-09-25T20:11:35Z

@BazookaJoe1900

Have you tried to read a packet which isn't on the master code? is it possible that you called it with wrong byte_count?
maybe you are working with something else then smart-battery?
I am working a lot with the SMBUS, and never got hardfault, so I wonder what is the difference

It happens for me when I read DeviceName (SBS Cmd 0x21) from a smart battery with a TI chipset.
I just inserted a bit of debug code to block_read:

	uint8_t device_address = get_device_address();
	rx_data[0] = (device_address << 1) | 0x00;
	rx_data[1] = cmd_code;
	rx_data[2] = (device_address << 1) | 0x01;
	byte_count = rx_data[3];

	PX4_ERR("Command: 0x%x, length: %d, byte_count: %d",cmd_code, length, byte_count);

And this is what I get:
ERROR [drivers__smbus] Command: 0x21, length: 8, byte_count: 13

The call returns the correct value for device name.

BazookaJoe1900 · 2020-09-25T20:45:37Z

@flochir

@BazookaJoe1900

Have you tried to read a packet which isn't on the master code? is it possible that you called it with wrong byte_count?
maybe you are working with something else then smart-battery?
I am working a lot with the SMBUS, and never got hardfault, so I wonder what is the difference

It happens for me when I read DeviceName (SBS Cmd 0x21) from a smart battery with a TI chipset.
I just inserted a bit of debug code to block_read:
	uint8_t device_address = get_device_address();
	rx_data[0] = (device_address << 1) | 0x00;
	rx_data[1] = cmd_code;
	rx_data[2] = (device_address << 1) | 0x01;
	byte_count = rx_data[3];

	PX4_ERR("Command: 0x%x, length: %d, byte_count: %d",cmd_code, length, byte_count);
And this is what I get:
ERROR [drivers__smbus] Command: 0x21, length: 8, byte_count: 13

The call returns the correct value for device name.

its seems that getting DeviceName length's should be 21bytes, and not 8.

My guess that byte_count is "only" 13 is because this is null terminated string, so TI cut the return to 13.
for some reason its not 'BQ40Z50'/'BQ40Z80' which it might be what you think it should get, can you confirm what you get when letting the byte_count to be 21?
anyway you did found serious bug here.

flochir · 2020-09-25T21:19:37Z

It's a custom BMS for Li-Ion we developed (also not based on BQ40Z50) so it doesn't return the default device string. I get the expected feedback from the device and it's working fine.

Just hit that bug while working on it and wanted to get it discussed/cleared.

jimdgit · 2020-09-26T03:13:57Z

A copy should always be length checked, but really on the surface it sounds like you only expected 8 bytes when you should have expected 13. No?

BazookaJoe1900 · 2020-09-26T07:23:43Z

so I think the bug understood, right?
is it ready to merge?
@flochir ? @bkueng ? @dagar ?

flochir · 2020-09-26T07:27:25Z

so I think the bug understood, right?
is it ready to merge?
@flochir ? @bkueng ? @dagar ?

Fine by me!

flochir · 2020-09-26T07:31:35Z

A copy should always be length checked, but really on the surface it sounds like you only expected 8 bytes when you should have expected 13. No?

In my case I only need the first 8 bytes. Of course that's how I hit this issue. It could happen with other batteries/chipsets anytime, though, even with a larger buffer.

flochir · 2020-09-26T07:40:04Z

Btw. there may be another issue to check. What happens if recv_len passed to I2C::transfer() is longer than the amount of data the SMBUS slave wants to transmit? Timeouts? Maybe there's some error or optimization potential here.

Side note: I did a quick test and requested a larger amount of data than returned through byte_count. Everything's stll working, so at least there don't seem to be any serious immediate problems.

* Added buffer length check to SMBus::block_read (triggered hardfault in batt_smbus module by writing beyond buffer end due to device returning longer byte_count than requested/expected) Co-authored-by: Florian Olbrich <florian.olbrich@scarabot.de> Co-authored-by: Daniel Agar <daniel@agar.ca> Co-authored-by: BazookaJoe1900 <BazookaJoe1900@gmail.com> Co-authored-by: Beat Küng <beat-kueng@gmx.net>

Added buffer length check to SMBus::block_read (triggered hardfault i…

6ad0654

…n batt_smbus module by writing beyond buffer end due to device returning longer byte_count than requested/expected)

dagar requested a review from BazookaJoe1900 September 22, 2020 12:37

dagar added the bug label Sep 22, 2020

dagar reviewed Sep 22, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.cpp Outdated Show resolved Hide resolved

flochir changed the title ~~Fix proposal for hardfault with battt_smbus module~~ Fix proposal for hardfault with batt_smbus module Sep 22, 2020

flochir and others added 2 commits September 22, 2020 14:40

Update src/lib/drivers/smbus/SMBus.cpp

73a67ca

Co-authored-by: Daniel Agar <daniel@agar.ca>

Follow up on update by dagar - removed unnecessary checks.

605bd85

BazookaJoe1900 added 2 commits September 22, 2020 23:36

compilation fix (partial)

e3684de

fix compilation issue for some boards

1dd3e30

bkueng reviewed Sep 23, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.cpp Outdated Show resolved Hide resolved

jimdgit reviewed Sep 23, 2020

View reviewed changes

BazookaJoe1900 previously approved these changes Sep 24, 2020

View reviewed changes

bkueng requested changes Sep 25, 2020

View reviewed changes

src/lib/drivers/smbus/SMBus.cpp Outdated Show resolved Hide resolved

limit byte_count to 32

988380d

Co-authored-by: Beat Küng <beat-kueng@gmx.net>

BazookaJoe1900 dismissed their stale review via 988380d September 25, 2020 10:51

bkueng previously approved these changes Sep 25, 2020

View reviewed changes

fix compilation

5728837

BazookaJoe1900 dismissed bkueng’s stale review via 5728837 September 25, 2020 14:07

BazookaJoe1900 self-requested a review September 26, 2020 09:11

BazookaJoe1900 approved these changes Sep 26, 2020

View reviewed changes

bkueng merged commit ec71088 into PX4:master Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix proposal for hardfault with batt_smbus module #15789

Fix proposal for hardfault with batt_smbus module #15789

flochir commented Sep 22, 2020

dagar commented Sep 22, 2020

BazookaJoe1900 commented Sep 22, 2020 •

edited

Loading

dagar commented Sep 23, 2020

jimdgit left a comment

BazookaJoe1900 commented Sep 24, 2020 •

edited

Loading

jimdgit commented Sep 24, 2020

BazookaJoe1900 commented Sep 24, 2020 •

edited

Loading

dagar commented Sep 24, 2020

jimdgit commented Sep 24, 2020

bkueng left a comment

flochir commented Sep 25, 2020 •

edited

Loading

jimdgit commented Sep 25, 2020 •

edited

Loading

flochir commented Sep 25, 2020

BazookaJoe1900 commented Sep 25, 2020 •

edited

Loading

flochir commented Sep 25, 2020

BazookaJoe1900 commented Sep 25, 2020

flochir commented Sep 25, 2020

jimdgit commented Sep 26, 2020

BazookaJoe1900 commented Sep 26, 2020

flochir commented Sep 26, 2020

flochir commented Sep 26, 2020

flochir commented Sep 26, 2020

Fix proposal for hardfault with batt_smbus module #15789

Fix proposal for hardfault with batt_smbus module #15789

Conversation

flochir commented Sep 22, 2020

dagar commented Sep 22, 2020

BazookaJoe1900 commented Sep 22, 2020 • edited Loading

dagar commented Sep 23, 2020

jimdgit left a comment

Choose a reason for hiding this comment

BazookaJoe1900 commented Sep 24, 2020 • edited Loading

jimdgit commented Sep 24, 2020

BazookaJoe1900 commented Sep 24, 2020 • edited Loading

dagar commented Sep 24, 2020

jimdgit commented Sep 24, 2020

bkueng left a comment

Choose a reason for hiding this comment

flochir commented Sep 25, 2020 • edited Loading

jimdgit commented Sep 25, 2020 • edited Loading

flochir commented Sep 25, 2020

BazookaJoe1900 commented Sep 25, 2020 • edited Loading

flochir commented Sep 25, 2020

BazookaJoe1900 commented Sep 25, 2020

flochir commented Sep 25, 2020

jimdgit commented Sep 26, 2020

BazookaJoe1900 commented Sep 26, 2020

flochir commented Sep 26, 2020

flochir commented Sep 26, 2020

flochir commented Sep 26, 2020

BazookaJoe1900 commented Sep 22, 2020 •

edited

Loading

BazookaJoe1900 commented Sep 24, 2020 •

edited

Loading

BazookaJoe1900 commented Sep 24, 2020 •

edited

Loading

flochir commented Sep 25, 2020 •

edited

Loading

jimdgit commented Sep 25, 2020 •

edited

Loading

BazookaJoe1900 commented Sep 25, 2020 •

edited

Loading