Make sure that files under bin/ directory of apk get X permission

This change adds support for adding executables under bin/ directory of
the apk, so that when that apk is mounted in the Microdroid the payload
can then execute such binaries.

As an example, put measure_io binary into the MicrodroidTestApp.apk, and
add a test that asserts that /mnt/apk/bin/measure_io has R & X
permissions.

In the following change the authfs benchmark tests will be migrated to
use the measure_io binary under /mnt/apk/bin/measure_io instead of
pushing one to the /data partition inside Microdroid. This is required
to reland the change that mounts /data with MS_NOEXEC.

Bug: 265261525
Bug: 270955654
Test: atest MicrodroidTestApp
Change-Id: Ia5294f2a1bc2a54505670425bbd835c7793c6f29
This commit is contained in:
Nikita Ioffe 2023-02-24 23:10:34 +00:00
parent 0dcd3f79ca
commit a7cb367145
8 changed files with 91 additions and 7 deletions

View File

@ -36,3 +36,19 @@ cc_binary {
"libbase",
],
}
// Package measure_io binary into a jar, to bundle with the MicrodroidTestApp.
// When MicrodroidTestApp is mounted inside the Microdroid, the zipfuse will
// add the +x permission on it.
java_genrule {
name: "measure_io_as_jar",
out: ["measure_io.jar"],
srcs: [
":measure_io",
],
cmd: "out_dir=$$(dirname $(out))" +
"&& bin_dir=\"bin\" " +
"&& mkdir -p $$out_dir/$$bin_dir" +
"&& cp $(in) $$out_dir/$$bin_dir" +
"&& jar cf $(out) -C $$out_dir $$bin_dir",
}

View File

@ -56,6 +56,9 @@ interface ITestService {
/* get the content of the specified file. */
String readFromFile(String path);
/* get file permissions of the give file by stat'ing it */
int getFilePermissions(String path);
/**
* Request the service to exit, triggering the termination of the VM. This may cause any
* requests in flight to fail.

View File

@ -443,6 +443,7 @@ public abstract class MicrodroidDeviceTestBase {
public String mFileContent;
public byte[] mBcc;
public long[] mTimings;
public int mFileMode;
public void assertNoException() {
if (mException != null) {

View File

@ -31,6 +31,7 @@ android_test {
"cbor-java",
"truth-prebuilt",
"compatibility-common-util-devicesidelib",
"measure_io_as_jar",
],
jni_libs: [
"MicrodroidTestNativeLib",

View File

@ -45,6 +45,7 @@ import android.os.ParcelFileDescriptor;
import android.os.ParcelFileDescriptor.AutoCloseInputStream;
import android.os.ParcelFileDescriptor.AutoCloseOutputStream;
import android.os.SystemProperties;
import android.system.OsConstants;
import android.system.virtualmachine.VirtualMachine;
import android.system.virtualmachine.VirtualMachineCallback;
import android.system.virtualmachine.VirtualMachineConfig;
@ -1779,6 +1780,42 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase {
}
}
@Test
@CddTest(requirements = {"9.17/C-1-5"})
public void testFileUnderBinHasExecutePermission() throws Exception {
assumeSupportedKernel();
VirtualMachineConfig vmConfig =
newVmConfigBuilder()
.setPayloadBinaryName("MicrodroidTestNativeLib.so")
.setMemoryBytes(minMemoryRequired())
.setDebugLevel(DEBUG_LEVEL_FULL)
.build();
VirtualMachine vm = forceCreateNewVirtualMachine("test_vm_perms", vmConfig);
TestResults testResults =
runVmTestService(
TAG,
vm,
(ts, tr) -> {
tr.mFileMode = ts.getFilePermissions("/mnt/apk/bin/measure_io");
});
testResults.assertNoException();
int allPermissionsMask =
OsConstants.S_IRUSR
| OsConstants.S_IWUSR
| OsConstants.S_IXUSR
| OsConstants.S_IRGRP
| OsConstants.S_IWGRP
| OsConstants.S_IXGRP
| OsConstants.S_IROTH
| OsConstants.S_IWOTH
| OsConstants.S_IXOTH;
assertThat(testResults.mFileMode & allPermissionsMask)
.isEqualTo(OsConstants.S_IRUSR | OsConstants.S_IXUSR);
}
private static class VmShareServiceConnection implements ServiceConnection {
private final CountDownLatch mLatch = new CountDownLatch(1);

View File

@ -251,6 +251,18 @@ Result<void> start_test_service() {
return ScopedAStatus::ok();
}
ScopedAStatus getFilePermissions(const std::string& path, int32_t* out) override {
struct stat sb;
if (stat(path.c_str(), &sb) != -1) {
*out = sb.st_mode;
} else {
std::string msg = "stat " + path + " failed : " + std::strerror(errno);
return ScopedAStatus::fromExceptionCodeWithMessage(EX_SERVICE_SPECIFIC,
msg.c_str());
}
return ScopedAStatus::ok();
}
ScopedAStatus quit() override { exit(0); }
};
auto testService = ndk::SharedRefBase::make<TestService>();

View File

@ -229,6 +229,11 @@ public class VmShareServiceImpl extends Service {
return mServiceInVm.readFromFile(path);
}
@Override
public int getFilePermissions(String path) throws RemoteException {
throw new UnsupportedOperationException("Not supported");
}
@Override
public void quit() throws RemoteException {
throw new UnsupportedOperationException("Not supported");

View File

@ -99,12 +99,8 @@ impl InodeData {
InodeData { mode, size: 0, data: InodeDataData::Directory(HashMap::new()) }
}
fn new_file(zip_index: ZipIndex, zip_file: &zip::read::ZipFile) -> InodeData {
InodeData {
mode: zip_file.unix_mode().unwrap_or(DEFAULT_FILE_MODE),
size: zip_file.size(),
data: InodeDataData::File(zip_index),
}
fn new_file(zip_index: ZipIndex, mode: u32, zip_file: &zip::read::ZipFile) -> InodeData {
InodeData { mode, size: zip_file.size(), data: InodeDataData::File(zip_index) }
}
fn add_to_directory(&mut self, name: CString, entry: DirectoryEntry) {
@ -188,6 +184,16 @@ impl InodeTable {
let mut parent = ROOT;
let mut iter = path.iter().peekable();
let mut file_mode = DEFAULT_FILE_MODE;
if path.starts_with("bin/") {
// Allow files under bin to have execute permission, this enables payloads to bundle
// additional binaries that they might want to execute.
// An example of such binary is measure_io one used in the authfs performance tests.
// More context available at b/265261525 and b/270955654.
file_mode |= libc::S_IXUSR;
}
while let Some(name) = iter.next() {
// TODO(jiyong): remove this check by canonicalizing `path`
if name == ".." {
@ -211,8 +217,11 @@ impl InodeTable {
}
// No inode found. Create a new inode and add it to the inode table.
// At the moment of writing this comment the apk file doesn't specify any
// permissions (apart from the ones on lib/), but it might change in the future.
// TODO(b/270955654): should we control the file permissions ourselves?
let inode = if is_file {
InodeData::new_file(i, &file)
InodeData::new_file(i, file.unix_mode().unwrap_or(file_mode), &file)
} else if is_leaf {
InodeData::new_dir(file.unix_mode().unwrap_or(DEFAULT_DIR_MODE))
} else {