Skip to content

Commit 750d69b

Browse files
author
Steven Moreland
committed
libbinder: remove overeager fdsan crash
In the continueWrite case, if a Parcel contians FDs, we try to tag the FDs we own in Parcel a second time. This causes a crash. Bug: 384097481 Test: with fuzzer, on device and host Ignore-AOSP-First: fuzzing Change-Id: I01b2312c316fa457781a4a0c1a81bfd845d0e60e
1 parent 575fdb3 commit 750d69b

2 files changed

Lines changed: 15 additions & 15 deletions

File tree

libs/binder/Parcel.cpp

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ enum {
156156

157157
#ifdef BINDER_WITH_KERNEL_IPC
158158
static void acquire_object(const sp<ProcessState>& proc, const flat_binder_object& obj,
159-
const void* who) {
159+
const void* who, bool tagFds) {
160160
switch (obj.hdr.type) {
161161
case BINDER_TYPE_BINDER:
162162
if (obj.binder) {
@@ -173,7 +173,7 @@ static void acquire_object(const sp<ProcessState>& proc, const flat_binder_objec
173173
return;
174174
}
175175
case BINDER_TYPE_FD: {
176-
if (obj.cookie != 0) { // owned
176+
if (tagFds && obj.cookie != 0) { // owned
177177
FdTag(obj.handle, nullptr, who);
178178
}
179179
return;
@@ -611,7 +611,7 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) {
611611
}
612612
}
613613

614-
acquire_object(proc, *flat, this);
614+
acquire_object(proc, *flat, this, true /*tagFds*/);
615615
}
616616
}
617617
#else
@@ -1797,7 +1797,7 @@ status_t Parcel::writeObject(const flat_binder_object& val, bool nullMetaData)
17971797
// Need to write meta-data?
17981798
if (nullMetaData || val.binder != 0) {
17991799
kernelFields->mObjects[kernelFields->mObjectsSize] = mDataPos;
1800-
acquire_object(ProcessState::self(), val, this);
1800+
acquire_object(ProcessState::self(), val, this, true /*tagFds*/);
18011801
kernelFields->mObjectsSize++;
18021802
}
18031803

@@ -2883,15 +2883,17 @@ void Parcel::releaseObjects()
28832883
#endif // BINDER_WITH_KERNEL_IPC
28842884
}
28852885

2886-
void Parcel::acquireObjects()
2887-
{
2886+
void Parcel::reacquireObjects(size_t objectsSize) {
28882887
auto* kernelFields = maybeKernelFields();
28892888
if (kernelFields == nullptr) {
28902889
return;
28912890
}
28922891

28932892
#ifdef BINDER_WITH_KERNEL_IPC
2894-
size_t i = kernelFields->mObjectsSize;
2893+
LOG_ALWAYS_FATAL_IF(objectsSize > kernelFields->mObjectsSize,
2894+
"Object size %zu out of range of %zu", objectsSize,
2895+
kernelFields->mObjectsSize);
2896+
size_t i = objectsSize;
28952897
if (i == 0) {
28962898
return;
28972899
}
@@ -2901,8 +2903,10 @@ void Parcel::acquireObjects()
29012903
while (i > 0) {
29022904
i--;
29032905
const flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
2904-
acquire_object(proc, *flat, this);
2906+
acquire_object(proc, *flat, this, false /*tagFds*/); // they are already tagged
29052907
}
2908+
#else
2909+
(void) objectsSize;
29062910
#endif // BINDER_WITH_KERNEL_IPC
29072911
}
29082912

@@ -3119,12 +3123,8 @@ status_t Parcel::continueWrite(size_t desired)
31193123
return NO_MEMORY;
31203124
}
31213125

3122-
// Little hack to only acquire references on objects
3123-
// we will be keeping.
3124-
size_t oldObjectsSize = kernelFields->mObjectsSize;
3125-
kernelFields->mObjectsSize = objectsSize;
3126-
acquireObjects();
3127-
kernelFields->mObjectsSize = oldObjectsSize;
3126+
// only acquire references on objects we are keeping
3127+
reacquireObjects(objectsSize);
31283128
}
31293129
if (rpcFields) {
31303130
if (status_t status = truncateRpcObjects(objectsSize); status != OK) {

libs/binder/include/binder/Parcel.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -672,7 +672,7 @@ class Parcel {
672672

673673
status_t finishWrite(size_t len);
674674
void releaseObjects();
675-
void acquireObjects();
675+
void reacquireObjects(size_t objectSize);
676676
status_t growData(size_t len);
677677
// Clear the Parcel and set the capacity to `desired`.
678678
// Doesn't reset the RPC session association.

0 commit comments

Comments
 (0)