[clang][analyzer] MmapWriteExecChecker improvements #97078
Merged
Conversation
This file contains hidden or 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
Read the 'mmap' flags from macro values and use a better test for the error situation .
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesRead the 'mmap' flags from macro values and use a better test for the error situation. Full diff: https://github.com/llvm/llvm-project/pull/97078.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
index cd1dd1b2fc511..a5a2bd62b47d6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
@@ -21,30 +21,55 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
using namespace clang;
using namespace ento;
namespace {
-class MmapWriteExecChecker : public Checker<check::PreCall> {
+class MmapWriteExecChecker
+ : public Checker<check::BeginFunction, check::PreCall> {
CallDescription MmapFn{CDM::CLibrary, {"mmap"}, 6};
CallDescription MprotectFn{CDM::CLibrary, {"mprotect"}, 3};
- static int ProtWrite;
- static int ProtExec;
- static int ProtRead;
const BugType BT{this, "W^X check fails, Write Exec prot flags set",
"Security"};
+ mutable bool FlagsInitialized = false;
+ mutable int ProtRead = 0x01;
+ mutable int ProtWrite = 0x02;
+ mutable int ProtExec = 0x04;
+
public:
+ void checkBeginFunction(CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
int ProtExecOv;
int ProtReadOv;
};
}
-int MmapWriteExecChecker::ProtWrite = 0x02;
-int MmapWriteExecChecker::ProtExec = 0x04;
-int MmapWriteExecChecker::ProtRead = 0x01;
+void MmapWriteExecChecker::checkBeginFunction(CheckerContext &C) const {
+ if (FlagsInitialized)
+ return;
+
+ FlagsInitialized = true;
+
+ const std::optional<int> FoundProtRead =
+ tryExpandAsInteger("PROT_READ", C.getPreprocessor());
+ const std::optional<int> FoundProtWrite =
+ tryExpandAsInteger("PROT_WRITE", C.getPreprocessor());
+ const std::optional<int> FoundProtExec =
+ tryExpandAsInteger("PROT_EXEC", C.getPreprocessor());
+ if (FoundProtRead && FoundProtWrite && FoundProtExec) {
+ ProtRead = *FoundProtRead;
+ ProtWrite = *FoundProtWrite;
+ ProtExec = *FoundProtExec;
+ } else {
+ // FIXME: Are these useful?
+ ProtRead = ProtReadOv;
+ ProtExec = ProtExecOv;
+ }
+}
void MmapWriteExecChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
@@ -54,16 +79,8 @@ void MmapWriteExecChecker::checkPreCall(const CallEvent &Call,
if (!ProtLoc)
return;
int64_t Prot = ProtLoc->getValue().getSExtValue();
- if (ProtExecOv != ProtExec)
- ProtExec = ProtExecOv;
- if (ProtReadOv != ProtRead)
- ProtRead = ProtReadOv;
-
- // Wrong settings
- if (ProtRead == ProtExec)
- return;
- if ((Prot & (ProtWrite | ProtExec)) == (ProtWrite | ProtExec)) {
+ if ((Prot & ProtWrite) && (Prot & ProtExec)) {
ExplodedNode *N = C.generateNonFatalErrorNode();
if (!N)
return;
diff --git a/clang/test/Analysis/mmap-writeexec.c b/clang/test/Analysis/mmap-writeexec.c
index 8fd86ceb9d2a2..88fcc7788e683 100644
--- a/clang/test/Analysis/mmap-writeexec.c
+++ b/clang/test/Analysis/mmap-writeexec.c
@@ -1,13 +1,14 @@
// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=alpha.security.MmapWriteExec -analyzer-config alpha.security.MmapWriteExec:MmapProtExec=1 -analyzer-config alpha.security.MmapWriteExec:MmapProtRead=4 -DUSE_ALTERNATIVE_PROT_EXEC_DEFINITION -verify %s
// RUN: %clang_analyze_cc1 -triple x86_64-unknown-apple-darwin10 -analyzer-checker=alpha.security.MmapWriteExec -verify %s
-#define PROT_WRITE 0x02
#ifndef USE_ALTERNATIVE_PROT_EXEC_DEFINITION
-#define PROT_EXEC 0x04
-#define PROT_READ 0x01
-#else
#define PROT_EXEC 0x01
+#define PROT_WRITE 0x02
#define PROT_READ 0x04
+#else
+#define PROT_EXEC 0x08
+#define PROT_WRITE 0x04
+#define PROT_READ 0x02
#endif
#define MAP_PRIVATE 0x0002
#define MAP_ANON 0x1000
|
Member
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesRead the 'mmap' flags from macro values and use a better test for the error situation. Full diff: https://github.com/llvm/llvm-project/pull/97078.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
index cd1dd1b2fc511..a5a2bd62b47d6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
@@ -21,30 +21,55 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
using namespace clang;
using namespace ento;
namespace {
-class MmapWriteExecChecker : public Checker<check::PreCall> {
+class MmapWriteExecChecker
+ : public Checker<check::BeginFunction, check::PreCall> {
CallDescription MmapFn{CDM::CLibrary, {"mmap"}, 6};
CallDescription MprotectFn{CDM::CLibrary, {"mprotect"}, 3};
- static int ProtWrite;
- static int ProtExec;
- static int ProtRead;
const BugType BT{this, "W^X check fails, Write Exec prot flags set",
"Security"};
+ mutable bool FlagsInitialized = false;
+ mutable int ProtRead = 0x01;
+ mutable int ProtWrite = 0x02;
+ mutable int ProtExec = 0x04;
+
public:
+ void checkBeginFunction(CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
int ProtExecOv;
int ProtReadOv;
};
}
-int MmapWriteExecChecker::ProtWrite = 0x02;
-int MmapWriteExecChecker::ProtExec = 0x04;
-int MmapWriteExecChecker::ProtRead = 0x01;
+void MmapWriteExecChecker::checkBeginFunction(CheckerContext &C) const {
+ if (FlagsInitialized)
+ return;
+
+ FlagsInitialized = true;
+
+ const std::optional<int> FoundProtRead =
+ tryExpandAsInteger("PROT_READ", C.getPreprocessor());
+ const std::optional<int> FoundProtWrite =
+ tryExpandAsInteger("PROT_WRITE", C.getPreprocessor());
+ const std::optional<int> FoundProtExec =
+ tryExpandAsInteger("PROT_EXEC", C.getPreprocessor());
+ if (FoundProtRead && FoundProtWrite && FoundProtExec) {
+ ProtRead = *FoundProtRead;
+ ProtWrite = *FoundProtWrite;
+ ProtExec = *FoundProtExec;
+ } else {
+ // FIXME: Are these useful?
+ ProtRead = ProtReadOv;
+ ProtExec = ProtExecOv;
+ }
+}
void MmapWriteExecChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
@@ -54,16 +79,8 @@ void MmapWriteExecChecker::checkPreCall(const CallEvent &Call,
if (!ProtLoc)
return;
int64_t Prot = ProtLoc->getValue().getSExtValue();
- if (ProtExecOv != ProtExec)
- ProtExec = ProtExecOv;
- if (ProtReadOv != ProtRead)
- ProtRead = ProtReadOv;
-
- // Wrong settings
- if (ProtRead == ProtExec)
- return;
- if ((Prot & (ProtWrite | ProtExec)) == (ProtWrite | ProtExec)) {
+ if ((Prot & ProtWrite) && (Prot & ProtExec)) {
ExplodedNode *N = C.generateNonFatalErrorNode();
if (!N)
return;
diff --git a/clang/test/Analysis/mmap-writeexec.c b/clang/test/Analysis/mmap-writeexec.c
index 8fd86ceb9d2a2..88fcc7788e683 100644
--- a/clang/test/Analysis/mmap-writeexec.c
+++ b/clang/test/Analysis/mmap-writeexec.c
@@ -1,13 +1,14 @@
// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=alpha.security.MmapWriteExec -analyzer-config alpha.security.MmapWriteExec:MmapProtExec=1 -analyzer-config alpha.security.MmapWriteExec:MmapProtRead=4 -DUSE_ALTERNATIVE_PROT_EXEC_DEFINITION -verify %s
// RUN: %clang_analyze_cc1 -triple x86_64-unknown-apple-darwin10 -analyzer-checker=alpha.security.MmapWriteExec -verify %s
-#define PROT_WRITE 0x02
#ifndef USE_ALTERNATIVE_PROT_EXEC_DEFINITION
-#define PROT_EXEC 0x04
-#define PROT_READ 0x01
-#else
#define PROT_EXEC 0x01
+#define PROT_WRITE 0x02
#define PROT_READ 0x04
+#else
+#define PROT_EXEC 0x08
+#define PROT_WRITE 0x04
+#define PROT_READ 0x02
#endif
#define MAP_PRIVATE 0x0002
#define MAP_ANON 0x1000
|
steakhal
requested changes
Jul 1, 2024
Contributor
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
Looks good. I only had some minor remarks.
NagyDonat
approved these changes
Jul 1, 2024
Contributor
There was a problem hiding this comment.
LGTM, straightforward change. Thanks!
EDIT: I posted this review without noticing that in the meantime @steakhal did his own review with some valuable suggestions. I'd still say that the PR would be acceptable without those stylistic tweaks, but if you apply them, then it'll be even better.
| ProtWrite = *FoundProtWrite; | ||
| ProtExec = *FoundProtExec; | ||
| } else { | ||
| // FIXME: Are these useful? |
Contributor
There was a problem hiding this comment.
They are not too useful (and it's a little bit surprising that there is no "ProtWriteOv"), but they're harmless and they don't require complex code, so let's keep them for now.
Collaborator
Author
|
I removed the options to specify |
steakhal
approved these changes
Jul 24, 2024
Contributor
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Nice cleanup.
steakhal
requested changes
Jul 24, 2024
clang/test/Analysis/mmap-writeexec.c
Outdated
| @@ -1,13 +1,14 @@ | |||
| // RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=alpha.security.MmapWriteExec -analyzer-config alpha.security.MmapWriteExec:MmapProtExec=1 -analyzer-config alpha.security.MmapWriteExec:MmapProtRead=4 -DUSE_ALTERNATIVE_PROT_EXEC_DEFINITION -verify %s | |||
Contributor
There was a problem hiding this comment.
Hmm, isn't the -analyzer-config alpha.security.MmapWriteExec:MmapProtRead=4 obsolete?
Why does this not error out with such a config?
Collaborator
Author
There was a problem hiding this comment.
There was really a test failure, fixed now.
steakhal
approved these changes
Jul 25, 2024
Contributor
|
Please make sure that the premerge bots are happy before merging. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/1169 Here is the relevant piece of the build log for the reference: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Read the 'mmap' flags from macro values and use a better test for the error situation.