Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A possible buffer overflow in I2C equestFrom()? #813

Closed
zapta opened this issue Dec 28, 2023 · 2 comments · Fixed by #872
Closed

A possible buffer overflow in I2C equestFrom()? #813

zapta opened this issue Dec 28, 2023 · 2 comments · Fixed by #872
Assignees
Labels
bug Something isn't working

Comments

@zapta
Copy link

zapta commented Dec 28, 2023

The relevant code is in the link below. The buffer size is 256 but the the method accepts a size_t len. What happens if len is greater than 256?

https://github.com/arduino/ArduinoCore-mbed/blob/main/libraries/Wire/Wire.cpp#L94

size_t arduino::MbedI2C::requestFrom(uint8_t address, size_t len, bool stopBit) {
	char buf[256];
	int ret = master->read(address << 1, buf, len, !stopBit);
	if (ret != 0) {
		return 0;
	}
	for (size_t i=0; i<len; i++) {
		rxBuffer.store_char(buf[i]);
	}
	return len;
}
@multiplemonomials
Copy link

Yeah this looks like a buffer overflow to me.
And actually, looking at the code, I think that Wire could be changed to use Mbed's single-byte I2C API. This would mean that the we wouldn't read the bytes and buffer them, we would instead read each byte one-by-one in arduino::MbedI2C::read().

@zapta
Copy link
Author

zapta commented Jan 7, 2024

I think that limiting the read count to 256 should be fine.

requestFrom() is supposed to return a byte so 255 max but 256 is more intuitive when reading in chunks or pages.

https://www.arduino.cc/reference/en/language/functions/communication/wire/requestfrom/

@iabdalkader iabdalkader self-assigned this Apr 19, 2024
@iabdalkader iabdalkader added the bug Something isn't working label Apr 19, 2024
iabdalkader added a commit to iabdalkader/ArduinoCore-mbed that referenced this issue Apr 19, 2024
Fixes arduino#813.

Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
iabdalkader added a commit to iabdalkader/ArduinoCore-mbed that referenced this issue Apr 19, 2024
Fixes arduino#813

Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants