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.

Sign up for GitHub

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

Jump to bottom

Fuzzing bug fixes #3989

Open
wants to merge 759 commits into
base: main
Choose a base branch
from
Open

Fuzzing bug fixes #3989

wants to merge 759 commits into from
+1,918 −779

Conversation

shankarseal
Copy link
Collaborator

@shankarseal shankarseal commented Nov 8, 2024 •
edited
Loading

Description

This PR fixes #4010. These are a collection of bugs found by fuzz testing.
This PR also fixes #3992, #3998 and #4001.

Testing

New fuzz tests were added.

Documentation

No.

Installation

No.

gtrevi and others added 30 commits January 23, 2024 15:00
@gtrevi
wip
04e5ce0
@gtrevi
wip
496d279
@gtrevi
Merge branch 'main' into gtrevi/vm-extensipn-update
5bb528d
@saxena-anurag
Merged PR 28837: Fix CICD
f87ea72 
Fix CICD build failures.
@gtrevi
Merge branch 'main' into gtrevi/vm-extensipn-update
f65e833
@gtrevi
wip
2fe1d1e
@gtrevi
wip
e7311fe
@gtrevi
Revert "wip"
51d637b 
This reverts commit e7311fe.
@gtrevi
sync
98db02d
@gtrevi
sync
03d2bd5
@gtrevi
sync
7c4f9bc
Merge remote-tracking branch 'upstream/main'
fb515e4
@gtrevi
feedback
cc3e409
@gtrevi
Merge branch 'gtrevi/vm-extensipn-update' of https://mscodehub.visual…
bbaf4ed 
…studio.com/eBPFForWindows/_git/eBPFForWindows into gtrevi/vm-extensipn-update
Merge remote-tracking branch 'upstream/main'
167bb94
Merge remote-tracking branch 'upstream/main'
ed9a947
Merge remote-tracking branch 'upstream/main'
88d7bb1
@gtrevi
draft
08b3f8c
@gtrevi
sync
be7286c
@gtrevi
sync
7b95e49
@gtrevi
draft
6907587
@gtrevi
Merge branch 'gtrevi/update-pipeline-for-vmextension' of https://msco…
c098c75 
…dehub.visualstudio.com/eBPFForWindows/_git/eBPFForWindows into gtrevi/update-pipeline-for-vmextension
@gtrevi
Merged PR 28028: Change VM Extension upgrade behavior to meet new Aut…
59e0b04 
…o-Update requirements.

This PR changes the following with the eBPF VM Extension:

- Adds workarounds for handling the new Auto-Update requirements with the existing VM Platform context handling limitations:
  - The *Update command* now handles atomically the entire updating process.
  - Before running, the *Update command* backs-up the current installation (can be the initial WinPA provisioning, or from a previous VM Extension).
  - Adds handling of a local "Updating" persistent state (as a registry key), in order to compensate the lack of a global state handled by the VM Extension platform.
  - The *Update operation* will fail the overall update if restarting the GuestProxyAgent service fails and, upon any failure, it will attempt to rollback to the previous installation.
    >Background: although restarting the GuestProxyAgent service is an extended operation that is not part of the eBPF VM Extension's scope, we should account success/failure of this operation in the overall update status, so that the VM Agent will stop rolling out updates to Azure VMs.
  - Allows the update to downgrade the current installation only if in the context of a rollback. Any other downgrade attempt will fail the *Update operation*.
- Adds more error return codes, defined in global constants for improved logging and troubleshooting.

Related work items: #158377
@gtrevi
Merge branch 'main' into gtrevi/update-pipeline-for-vmextension
48b7b21
@gtrevi
wip
bc8f5a7
@gtrevi
nit
5ce9bb9
@gtrevi
wip
c359e91
@gtrevi
wip
5e4f8a7
@gtrevi
wip
e8adeba
@gtrevi
wip
fa2b4a7
dthaler
dthaler reviewed Nov 8, 2024
View reviewed changes
libs/execution_context/ebpf_core.c Outdated Show resolved Hide resolved
libs/execution_context/ebpf_program.c Outdated Show resolved Hide resolved
libs/runtime/ebpf_bitmap.c Outdated Show resolved Hide resolved
libs/runtime/ebpf_bitmap.h Show resolved Hide resolved
scripts/config_test_vm.psm1 Outdated Show resolved Hide resolved
tests/libfuzzer/include/fuzz_helper_function.hpp Outdated Show resolved Hide resolved
tests/libfuzzer/include/fuzz_helper_function.hpp Outdated Show resolved Hide resolved
tests/libfuzzer/include/fuzz_helper_function.hpp Outdated Show resolved Hide resolved
tests/libfuzzer/include/fuzz_helper_function.hpp Outdated Show resolved Hide resolved
tests/sample/sample.vcxproj.filters Outdated Show resolved Hide resolved
@Alan-Jowett
Copy link
Member

In my view "These are a collection of bug fixes found by fuzz testing." is not a sufficient PR description. I don't know how to review this since I don't know what's being fixed so I can't tell whether it fixes them right. (And I suspect not since the tests appear to be failing.)

I second this. As much as it is going to be painful to do, we should file each internally found bug in github, and fix them in the open.

@Alan-Jowett Alan-Jowett self-requested a review November 9, 2024 07:54
@shankarseal shankarseal force-pushed the fuzzing_bug_fixes branch 3 times, most recently from 6b66335 to 9b89e9e Compare November 14, 2024 02:24
@dthaler
Copy link
Collaborator

dthaler commented Nov 14, 2024

Issue #4001 is fixed by PR #4002 so I'd recommend that be merged first and this be rebased, or else remove it from this PR.

@dthaler
Copy link
Collaborator

dthaler commented Nov 14, 2024

Issue #3992 is fixed by PR #3991 so I'd recommend that be merged first and this be rebased, or else remove it from this PR.

@dthaler dthaler added the security Related to security hardening label Nov 14, 2024
@dthaler
Copy link
Collaborator

dthaler commented Nov 14, 2024

Issue #3998 is just a perf optimization according to that issue, and is apparently non-trivial, so should be separated from security bug fixes. The principle is that a fix for #4010 would be a backport candidate (e.g., to v.0.19.1) but a PR for #3998 would not be since it doesn't fix any security issue that affects production.

@shankarseal
Copy link
Collaborator Author

shankarseal commented Nov 15, 2024 •
edited
Loading

Issue #3998 is just a perf optimization according to that issue, and is apparently non-trivial, so should be separated from security bug fixes. The principle is that a fix for #4010 would be a backport candidate (e.g., to v.0.19.1) but a PR for #3998 would not be since it doesn't fix any security issue that affects production.

0.19.1 already has these changes that are being merged to main. Without fix to 3998 first, many of the issues would not be even found, that this PR is fixing as otherwise the fuzz tests took too long to run and often timed out without finding any issues.

Hopefully, this is the first and last time where we have to merge so many fixes together. Please resolve this comment so that we can get this important payload merged.

@shankarseal
Copy link
Collaborator Author

In my view "These are a collection of bug fixes found by fuzz testing." is not a sufficient PR description. I don't know how to review this since I don't know what's being fixed so I can't tell whether it fixes them right. (And I suspect not since the tests appear to be failing.)

I second this. As much as it is going to be painful to do, we should file each internally found bug in github, and fix them in the open.

Due to special circumstances we had to take these set of changes this way, after discussing with Microsoft security experts. Hopefully we will only rarely get security bugs, which we will fix according to the MSRC process.

@shankarseal
Copy link
Collaborator Author

Issue #4001 is fixed by PR #4002 so I'd recommend that be merged first and this be rebased, or else remove it from this PR.

#4002 is changed to draft.

@shankarseal
Copy link
Collaborator Author

Issue #3992 is fixed by PR #3991 so I'd recommend that be merged first and this be rebased, or else remove it from this PR.

#3991 is changed to draft.

@shankarseal
Copy link
Collaborator Author

In my view "These are a collection of bug fixes found by fuzz testing." is not a sufficient PR description. I don't know how to review this since I don't know what's being fixed so I can't tell whether it fixes them right. (And I suspect not since the tests appear to be failing.)

PR description is updated with more context and a new issue #4010 is created that lists all the defects found by fuzzing

@shankarseal shankarseal added this pull request to the merge queue Nov 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2024
@Alan-Jowett Alan-Jowett added this pull request to the merge queue Nov 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2024
@shankarseal shankarseal added this pull request to the merge queue Nov 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dthaler dthaler dthaler left review comments

@Alan-Jowett Alan-Jowett Alan-Jowett approved these changes

@mikeagun mikeagun mikeagun approved these changes

@poornagmsft poornagmsft Awaiting requested review from poornagmsft poornagmsft is a code owner

@saxena-anurag saxena-anurag Awaiting requested review from saxena-anurag saxena-anurag is a code owner

@dv-msft dv-msft Awaiting requested review from dv-msft dv-msft is a code owner

@gtrevi gtrevi Awaiting requested review from gtrevi gtrevi is a code owner

@shpalani shpalani Awaiting requested review from shpalani shpalani is a code owner

@matthewige matthewige Awaiting requested review from matthewige matthewige is a code owner

@mtfriesen mtfriesen Awaiting requested review from mtfriesen mtfriesen is a code owner

@rectified95 rectified95 Awaiting requested review from rectified95 rectified95 is a code owner

Assignees
No one assigned
Labels
security Related to security hardening
Projects
None yet
Milestone
No milestone
10 participants
@shankarseal @Alan-Jowett @dthaler @mikeagun @gtrevi @saxena-anurag @matthewige @ben-zen @shpalani @mtfriesen