Fix dhcp option buffer issue #12033
Merged
Fix dhcp option buffer issue #12033
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Gang Lv ganglv@microsoft.com
|
Possible to test in KVM? |
I have used kvm and sonic-mgmt to verify this issue. |
|
Please merge it together with sonic-mgmt PR. |
5 tasks
|
Add end2end test: |
qiluo-msft
added
Request for 201911 Branch 
Request for 202006 Branch
Request for 202012 Branch
Request for 202106 Branch
Request for 202111 Branch
kellyyeh
approved these changes
Sep 15, 2022
qiluo-msft
pushed a commit
that referenced
this pull request
Sep 17, 2022
Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com
yxieca
pushed a commit
that referenced
this pull request
Sep 21, 2022
Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com
ganglyu
added a commit
to sonic-net/sonic-mgmt
that referenced
this pull request
Sep 26, 2022
Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
wangxin
pushed a commit
to sonic-net/sonic-mgmt
that referenced
this pull request
Sep 28, 2022
Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
wangxin
pushed a commit
to sonic-net/sonic-mgmt
that referenced
this pull request
Sep 28, 2022
Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
mssonicbld
pushed a commit
to mssonicbld/sonic-buildimage
that referenced
this pull request
Oct 13, 2022
Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com
|
Cherry-pick PR to 202111: #12385 |
liushilongbuaa
pushed a commit
that referenced
this pull request
Oct 16, 2022
Why I did it Current isc-dhcp uses below code to remove DHCP option: memmove(sp, op, op[1] + 2); sp += op[1] + 2; sp points to the option to be stripped, we can call it as option S. op points to the option after options S, we can call it as option O. DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value. In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result: Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove. But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp. How I did it Create patch from https://github.com/isc-projects/dhcp The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug. size_t mlen = op[1] + 2; memmove(sp, op, mlen); sp += mlen; How to verify it I have a PR for sonic-mgmt to cover this issue: sonic-net/sonic-mgmt#6330 Signed-off-by: Gang Lv ganglv@microsoft.com Signed-off-by: Gang Lv ganglv@microsoft.com Co-authored-by: ganglv <88995770+ganglyu@users.noreply.github.com>
mssonicbld
added
Included in 202111 Branch
and removed
Request for 202111 Branch
Azarack
pushed a commit
to Azarack/sonic-mgmt
that referenced
this pull request
Oct 17, 2022
Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
This was referenced Oct 26, 2022
ganglyu
added a commit
that referenced
this pull request
Oct 28, 2022
Why I did it #12033 How I did it How to verify it
allen-xf
pushed a commit
to allen-xf/sonic-mgmt
that referenced
this pull request
Oct 28, 2022
Signed-off-by: Gang Lv ganglv@microsoft.com What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
ganglyu
added a commit
to sonic-net/sonic-mgmt
that referenced
this pull request
Apr 21, 2023
What is the motivation for this PR? dhcp relay has an option82 related pr, and we need to update e2e test to verify this issue: sonic-net/sonic-buildimage#12033 How did you do it? Update dhcp offer packet, add option82 and another option. After dhcp relay, option82 should be stripped. How did you verify/test it? I used kvm to verify this test case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Gang Lv ganglv@microsoft.com
Why I did it
Current isc-dhcp uses below code to remove DHCP option:
memmove(sp, op, op[1] + 2);sp += op[1] + 2;sp points to the option to be stripped, we can call it as option S.
op points to the option after options S, we can call it as option O.
DHCP option is a typical type-length-value structure, the first byte is type, the second byte is length, and remain parts are value.
In this case, option O length is bigger than option S, and more than 2 bytes, after the memmove, we will get this result:
Now Option S and Option O are overwritten, op[1] was the length of Option O, and it's modified after memmove.
But current implementation is still using op[1] as length to update sp (sp+=op[1]+2), so we get the wrong sp.
How I did it
Create patch from https://github.com/isc-projects/dhcp
The new impelementation use mlen to store the length of Option O before memmove, that's how it fixed the bug.
size_t mlen = op[1] + 2;memmove(sp, op, mlen);sp += mlen;How to verify it
I have a PR for sonic-mgmt to cover this issue:
sonic-net/sonic-mgmt#6330
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)