Tests for copyin(9) handling of kernel addresses
ClosedPublic
Actions

Authored by kib on Oct 17 2015, 10:41 AM.

Details

Reviewers

emaste
ngie

Commits

rS289603: Add tests for the copyin(9) handling of illegal buffers.

Summary

I wrote morally equivalent tests when I developed the arm patch r289372. The same issue is present on arm64.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib updated this revision to Diff 9463.Oct 17 2015, 10:41 AM

kib retitled this revision from to Tests for copyin(9) handling of kernel addresses.

kib updated this object.

kib edited the test plan for this revision. (Show Details)

kib added a reviewer: emaste.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

kib added a project: tests.

Herald added a subscriber: imp. · View Herald TranscriptOct 17 2015, 10:41 AM

Is it worth checking VM_MAXUSER_ADDRESS + 1, 0?

emaste accepted this revision.Oct 19 2015, 4:04 PM

emaste edited edge metadata.

emaste added subscribers: ngie, jmmv.

This revision is now accepted and ready to land.Oct 19 2015, 4:06 PM

In D3925#81835, @emaste wrote:

Is it worth checking VM_MAXUSER_ADDRESS + 1, 0?

(VM_MAXUSER_ADDRESS, 0) is only considered valid because the short-circuit for length == 0 is taken before the interval validation is performed. So, knowing the implementations, I would say that it should be excessive. Still, since the question was raised, I will add the test as well.

Add VM_MAXUSER + 1, 0 test.

This revision now requires review to proceed.Oct 19 2015, 4:29 PM

emaste accepted this revision.Oct 19 2015, 4:33 PM

emaste edited edge metadata.

This revision is now accepted and ready to land.Oct 19 2015, 4:33 PM

ngie added inline comments.Oct 19 2015, 6:11 PM

tests/sys/kern/kern_copyin.c
60–64	Creating a temporary file in /tmp violates ATF/kyua's sandbox, even though you call unlink after the fact. Please use `copyin.XXXXXX` instead
66–78	`ATF_CHECK(copyin_checker... == 0);` is the same thing. I don't know if jmmv had a preference but IIRC the more complex macros were going to be removed, maybe, someday (yes, I am being wishy-washy on purpose here).

ngie requested changes to this revision.Oct 19 2015, 6:11 PM

ngie added a reviewer: ngie.

This revision now requires changes to proceed.Oct 19 2015, 6:11 PM

Do not create temp file in /tmp. Use ATF_CHECK instead of ATF_CHECK_EQ.

Sidenote: the only thing that's slightly painful with ATF that other test infrastructures seem to do better is that ATF_CHECK/ATF_REQUIRE don't print out the result of the expression, so if you need to dump out the result of the check, use ATF_CHECK_MSG/ATF_REQUIRE_MSG instead, like:

int rc;

ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");

LGTM!

This revision is now accepted and ready to land.Oct 19 2015, 6:40 PM

emaste accepted this revision.Oct 19 2015, 6:41 PM

emaste edited edge metadata.

In D3925#81898, @ngie wrote:

int rc;

ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");

This is extremely ugly. For my tests, the function is the same, but the arguments are what produce the test case. In essense, I have to either retype the same thing twice, or create a wrapper that would call ATF_CHECK_MSG() only in case of failure.

I decided to keep this as is, since the most likely outcome of the failing test is the kernel panic, anyway.

In D3925#81904, @kib wrote:
In D3925#81898, @ngie wrote:
int rc;

ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");
This is extremely ugly. For my tests, the function is the same, but the arguments are what produce the test case. In essense, I have to either retype the same thing twice, or create a wrapper that would call ATF_CHECK_MSG() only in case of failure.