espressif
Buffer overrun in the I2C driver (IDFGH-14934)
Answers checklist.
- [x] I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
- [x] I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
- [x] I have searched the issue tracker for a similar issue and not found a similar issue.
General issue report
The s_i2c_handle_complete function is using the wrong limits when copying data out of the rxfifo: https://github.com/espressif/esp-idf/blob/23c73cdc37d9b8e6c840ea99c5ae12b9a0ce7032/components/esp_driver_i2c/i2c_slave.c#L76
The code is currently using t->rcv_fifo_cnt, but it should be using the rx_fifo_cnt, similar to how it is done in line 64 in the s_i2c_handle_rx_fifo_wm function. It should then also limit the number of bytes that are copied into the t->buffer.
If the i2c_slave_receive function is called with a receive_size of more than the hw-fifo (32 bytes), then the code currently overwrites part of the handle structure.
From what I can see this is not an issue in v2 anymore.
Yes and I'm sorry but we will remove v1 driver quickly in next version, so I'm going to close this issue and recommand you use v2 driver. Thanks
Yes and I'm sorry but we will remove v1 driver quickly in next version, so I'm going to close this issue and recommand you use v2 driver. Thanks
IMHO, you have to fix it if any of the esp-idf branches in maintenance period still allow using v1 driver.
@AxelLin TBH, the v1 logic is very un-natural, as fix a buffer issue cannot solve the very basic logic problme. So we have another set of api called v2. So that's why we want to stop maintain. Thanks for your understanding.
I partially agree with @AxelLin and partially with @mythbuster5.
A buffer-overrun is kind-of bad and hard to track down. So fixing it in the other branches would be nice. Especially, since it's just replacing lines 72 to 80 with an unconditional call to s_i2c_handle_rx_fifo_wm. (I think).
On the other hand the current i2c slave driver isn't really usable, and I would have needed to patch it anyway. So very few people would ever encounter it (if any).
I think the question is whether patching the old branches is hard/annoying or not. If it's just committing the fix, then I would probably do it. If you need to go through hoops, then it's not worth it.
@mythbuster5 so the maintenance period is binding until a subsystem maintainer decides it isn't? I don't care at all about the I2C Slave driver, but that's just wrong, and sets a bad precedent.
