Skip to content

Commit

Permalink
libhwbinder: support TF_CLEAR_BUF
Browse files Browse the repository at this point in the history
This flag instructs the kernel to clear transactions from send/reply
buffers for certain transactions which may contain sensitive data, as a
security precaution.

Bug: 171501998
Test: hidl_test + verify by reading memory bits w/ updated kernel
Change-Id: I7dda8f8d24091f77bdaf99a7de446875356c601c
  • Loading branch information
smore-lore committed Nov 20, 2020
1 parent 2f57140 commit a80270c
Show file tree
Hide file tree
Showing 9 changed files with 108 additions and 9 deletions.
1 change: 1 addition & 0 deletions Android.bp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ cc_defaults {
"ProcessState.cpp",
"Static.cpp",
"TextOutput.cpp",
"Utils.cpp",
],

product_variables: {
Expand Down
4 changes: 4 additions & 0 deletions Binder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ status_t BHwBinder::transact(
{
data.setDataPosition(0);

if (reply != nullptr && (flags & FLAG_CLEAR_BUF)) {
reply->markSensitive();
}

status_t err = NO_ERROR;
switch (code) {
default:
Expand Down
6 changes: 4 additions & 2 deletions IPCThreadState.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1170,6 +1170,8 @@ status_t IPCThreadState::executeCommand(int32_t cmd)
<< reinterpret_cast<const size_t*>(tr.data.ptr.offsets) << endl;
}

constexpr size_t kForwardReplyFlags = TF_CLEAR_BUF;

auto reply_callback = [&] (auto &replyParcel) {
if (reply_sent) {
// Reply was sent earlier, ignore it.
Expand All @@ -1179,7 +1181,7 @@ status_t IPCThreadState::executeCommand(int32_t cmd)
reply_sent = true;
if ((tr.flags & TF_ONE_WAY) == 0) {
replyParcel.setError(NO_ERROR);
sendReply(replyParcel, 0);
sendReply(replyParcel, (tr.flags & kForwardReplyFlags));
} else {
ALOGE("Not sending reply in one-way transaction");
}
Expand All @@ -1206,7 +1208,7 @@ status_t IPCThreadState::executeCommand(int32_t cmd)
// Should have been a reply but there wasn't, so there
// must have been an error instead.
reply.setError(error);
sendReply(reply, 0);
sendReply(reply, (tr.flags & kForwardReplyFlags));
} else {
if (error != NO_ERROR) {
ALOGE("transact() returned error after sending reply.");
Expand Down
29 changes: 27 additions & 2 deletions Parcel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include "binder_kernel.h"
#include <hwbinder/Static.h>
#include "TextOutput.h"
#include "Utils.h"

#include <atomic>

Expand Down Expand Up @@ -355,6 +356,11 @@ status_t Parcel::setData(const uint8_t* buffer, size_t len)
return err;
}

void Parcel::markSensitive() const
{
mDeallocZero = true;
}

// Write RPC headers. (previously just the interface token)
status_t Parcel::writeInterfaceToken(const char* interface)
{
Expand Down Expand Up @@ -1689,6 +1695,9 @@ void Parcel::freeDataNoInit()
LOG_ALLOC("Parcel %p: freeing with %zu capacity", this, mDataCapacity);
gParcelGlobalAllocSize -= mDataCapacity;
gParcelGlobalAllocCount--;
if (mDeallocZero) {
zeroMemory(mData, mDataSize);
}
free(mData);
}
if (mObjects) free(mObjects);
Expand All @@ -1708,6 +1717,21 @@ status_t Parcel::growData(size_t len)
return continueWrite(newSize);
}

static uint8_t* reallocZeroFree(uint8_t* data, size_t oldCapacity, size_t newCapacity, bool zero) {
if (!zero) {
return (uint8_t*)realloc(data, newCapacity);
}
uint8_t* newData = (uint8_t*)malloc(newCapacity);
if (!newData) {
return nullptr;
}

memcpy(newData, data, std::min(oldCapacity, newCapacity));
zeroMemory(data, oldCapacity);
free(data);
return newData;
}

status_t Parcel::restartWrite(size_t desired)
{
if (desired > INT32_MAX) {
Expand All @@ -1721,7 +1745,7 @@ status_t Parcel::restartWrite(size_t desired)
return continueWrite(desired);
}

uint8_t* data = (uint8_t*)realloc(mData, desired);
uint8_t* data = reallocZeroFree(mData, mDataCapacity, desired, mDeallocZero);
if (!data && desired > mDataCapacity) {
mError = NO_MEMORY;
return NO_MEMORY;
Expand Down Expand Up @@ -1871,7 +1895,7 @@ status_t Parcel::continueWrite(size_t desired)

// We own the data, so we can just do a realloc().
if (desired > mDataCapacity) {
uint8_t* data = (uint8_t*)realloc(mData, desired);
uint8_t* data = reallocZeroFree(mData, mDataCapacity, desired, mDeallocZero);
if (data) {
LOG_ALLOC("Parcel %p: continue from %zu to %zu capacity", this, mDataCapacity,
desired);
Expand Down Expand Up @@ -1938,6 +1962,7 @@ void Parcel::initState()
mHasFds = false;
mFdsKnown = true;
mAllowFds = true;
mDeallocZero = false;
mOwner = nullptr;
clearCache();

Expand Down
27 changes: 27 additions & 0 deletions Utils.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "Utils.h"

#include <string.h>

namespace android::hardware {

void zeroMemory(uint8_t* data, size_t size) {
memset(data, 0, size);
}

} // namespace android::hardware
25 changes: 25 additions & 0 deletions Utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <cstdint>
#include <stddef.h>

namespace android::hardware {

// avoid optimizations
void zeroMemory(uint8_t* data, size_t size);

} // namespace android::hardware
8 changes: 4 additions & 4 deletions binder_kernel.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@
#ifndef ANDROID_HARDWARE_BINDER_KERNEL_H
#define ANDROID_HARDWARE_BINDER_KERNEL_H

/**
* Only need this file to fix the __packed__ keyword.
*/

// TODO(b/31559095): bionic on host
#ifndef __ANDROID__
#define __packed __attribute__((__packed__))
#endif

#include <linux/android/binder.h>

enum transaction_flags_ext {
TF_CLEAR_BUF = 0x20, /* clear buffer on txn complete */
};

#endif // ANDROID_HARDWARE_BINDER_KERNEL_H
6 changes: 5 additions & 1 deletion include/hwbinder/IBinder.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ class IBinder : public virtual RefBase
LAST_HIDL_TRANSACTION = 0x0fffffff,

// Corresponds to TF_ONE_WAY -- an asynchronous call.
FLAG_ONEWAY = 0x00000001
FLAG_ONEWAY = 0x00000001,

// Corresponds to TF_CLEAR_BUF -- clear transaction buffers after call
// is made
FLAG_CLEAR_BUF = 0x00000020,
};

IBinder();
Expand Down
11 changes: 11 additions & 0 deletions include/hwbinder/Parcel.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ class Parcel {

status_t setData(const uint8_t* buffer, size_t len);

// Zeros data when reallocating. Other mitigations may be added
// in the future.
//
// WARNING: some read methods may make additional copies of data.
// In order to verify this, heap dumps should be used.
void markSensitive() const;

// Writes the RPC header.
status_t writeInterfaceToken(const char* interface);

Expand Down Expand Up @@ -292,6 +299,10 @@ class Parcel {
mutable bool mHasFds;
bool mAllowFds;

// if this parcelable is involved in a secure transaction, force the
// data to be overridden with zero when deallocated
mutable bool mDeallocZero;

release_func mOwner;
void* mOwnerCookie;
};
Expand Down

0 comments on commit a80270c

Please sign in to comment.