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

Fix MRP to not happen over BTP again. #9792

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

We were incorrectly doing MRP over BTP, because there was no peer
connection state for our insecure session when doing a SPAKE
handshake.

The various header include additions were needed to fix header files
that used types without declaring them and hence depended on other
headers being included before them.

Fixes #9738

Problem

We are doing MRP over BTP, which is very broken because BTP modifies the message packetbuffer before sending it.

Change overview

Go back to not doing MRP over BTP.

Testing

Manually did ble-wifi pairing from chip-tool to an m5stack and verified that it works again.

We were incorrectly doing MRP over BTP, because there was no peer
connection state for our insecure session when doing a SPAKE
handshake.

The various header include additions were needed to fix header files
that used types without declaring them and hence depended on other
headers being included before them.

Fixes project-chip#9738
Copy link
Contributor

@msandstedt msandstedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, this fix is pretty obvious isn't it! But thank you for sorting it out.

I do have a question though. This brings up a bit of a concern for me about buffer ownership: obviously the BTP layer considers the transmit message buffer as 'owned' by it, and therefore assumes it can do in-place modification safely.

But is there a way for us to more cleanly track ownership in our stack? It would be nice if it were impossible for this to happen in the first place. Obviously MRP and BTP can't both own the buffer, and this totally breaks if they try to share the buffer and one of them modifies it. But shouldn't we have something in our stack that actively protects against such a thing?

Copy link
Contributor

@kghost kghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to extra a virtual interface from all session (UnauthenticatedSession, SecureSession, and future GroupSession), and make the SessionHandle behaves like a pointer to the session. I will try it next week.

Copy link
Contributor

@turon turon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hot fix. Had one high-level question regarding unsecured session handling.

src/transport/SessionHandle.cpp Show resolved Hide resolved
@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 747b485

File Section File VM
chip-qpg6100-lighting-example.out .text 112 112
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,44787
.debug_line,0,4263
.debug_abbrev,0,3256
.debug_loc,0,1498
.debug_ranges,0,376
.debug_str,0,172
.text,112,112
.strtab,0,83
.debug_frame,0,76
.symtab,0,48
.debug_aranges,0,40
.shstrtab,0,1
[Unmapped],0,-112

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'


@bzbarsky-apple
Copy link
Contributor Author

This brings up a bit of a concern for me about buffer ownership

Absolutely, @msandstedt

We have this as a long-standing issue with LwIP too, by the way, in that it modifies the buffers handed to it; the way that's handled is by having the send code, when backed by LwIP, always clone the buffer being sent before handing it off to LwIP...

Another option would be to have MRP itself make a copy of the data before sending. There were concerns about doing that and the ensuing "two copies of the data when we don't need it in configurations where the network layer does not modify the buffer"...

But yes, the current situation is pretty fragile. :(

@bzbarsky-apple
Copy link
Contributor Author

I think we need to extra a virtual interface from all session

It seems to me that we should have some sort of session interface yes, one where a session has enough state internally that we can just ask it questions. The fact that I had to pass in a SecureSessionMgr from the caller in this PR is pretty unfortunate, imo.

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 747b485

File Section File VM
chip-shell.elf text 112 112
chip-lock.elf text 108 108
chip-lock.elf device_handles -12 -12
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,44844
.debug_line,0,3670
.debug_abbrev,0,3406
.debug_loc,0,1110
.debug_ranges,0,376
.debug_str,0,134
text,112,112
.strtab,0,83
.debug_frame,0,76
.symtab,0,48
.debug_aranges,0,40
.shstrtab,0,1

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,45816
.debug_line,0,3666
.debug_abbrev,0,3406
.debug_loc,0,1094
.debug_ranges,0,376
.debug_str,0,134
text,108,108
.strtab,0,83
.debug_frame,0,76
.symtab,0,48
.debug_aranges,0,40
.shstrtab,0,1
device_handles,-12,-12


@github-actions
Copy link

Size increase report for "esp32-example-build" from 747b485

File Section File VM
chip-all-clusters-app.elf .flash.text 144 144
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,135552
.debug_line,0,5228
.debug_abbrev,0,4545
.debug_loc,0,711
.debug_ranges,0,320
.debug_str,0,172
.flash.text,144,144
.debug_frame,0,84
.strtab,0,83
.debug_aranges,0,40
.symtab,0,32
.shstrtab,0,1
[Unmapped],0,-144


@woody-apple woody-apple merged commit f880d00 into project-chip:master Sep 17, 2021
@bzbarsky-apple bzbarsky-apple deleted the fix-ble-commissioning branch September 17, 2021 17:47
mleisner pushed a commit to mleisner/connectedhomeip that referenced this pull request Sep 20, 2021
We were incorrectly doing MRP over BTP, because there was no peer
connection state for our insecure session when doing a SPAKE
handshake.

The various header include additions were needed to fix header files
that used types without declaring them and hence depended on other
headers being included before them.

Fixes project-chip#9738
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BLE commissioning fails after unauthenticated session changes
9 participants