| Message ID | 20190801092616.30553-3-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/9] lib: Add support for guarded buffers | expand |
----- Original Message ----- > In a case that the buffer size is not a multiple of a page size there is > unused space before the start of the buffer. Let's fill that with > center mirrored random bytes and check that the buffer wasn't modified > before we unmap it. > > void *tst_alloc(size_t size) > { > size_t page_size = getpagesize(); > @@ -34,9 +61,13 @@ void *tst_alloc(size_t size) > maps = map; > > if (size % page_size) > - ret += page_size - (size % page_size); > + map->buf_shift = page_size - (size % page_size); > + else > + map->buf_shift = 0; > + > + setup_canary(map); > > - return ret; > + return ret + map->buf_shift; My concern here is alignment.
Hi! > > In a case that the buffer size is not a multiple of a page size there is > > unused space before the start of the buffer. Let's fill that with > > center mirrored random bytes and check that the buffer wasn't modified > > before we unmap it. > > > > void *tst_alloc(size_t size) > > { > > size_t page_size = getpagesize(); > > @@ -34,9 +61,13 @@ void *tst_alloc(size_t size) > > maps = map; > > > > if (size % page_size) > > - ret += page_size - (size % page_size); > > + map->buf_shift = page_size - (size % page_size); > > + else > > + map->buf_shift = 0; > > + > > + setup_canary(map); > > > > - return ret; > > + return ret + map->buf_shift; > > My concern here is alignment. I'm aware of that. My reasoning here is that: * The end of the page is aligned by definition to 2^page_order * Any primitive types such as integer, etc. are hence aligned * Structures are padded so that the total size is multiple of the largest alignment required (because otherwise arrays of structures would end up causing unaligned access as well). That leaves out things such as buffers for direct I/O, the only way to allocate aligned buffers there is to make the size to be multiple of the block size.
----- Original Message ----- > Hi! > > > In a case that the buffer size is not a multiple of a page size there is > > > unused space before the start of the buffer. Let's fill that with > > > center mirrored random bytes and check that the buffer wasn't modified > > > before we unmap it. > > > > > > void *tst_alloc(size_t size) > > > { > > > size_t page_size = getpagesize(); > > > @@ -34,9 +61,13 @@ void *tst_alloc(size_t size) > > > maps = map; > > > > > > if (size % page_size) > > > - ret += page_size - (size % page_size); > > > + map->buf_shift = page_size - (size % page_size); > > > + else > > > + map->buf_shift = 0; > > > + > > > + setup_canary(map); > > > > > > - return ret; > > > + return ret + map->buf_shift; > > > > My concern here is alignment. > > I'm aware of that. My reasoning here is that: > > * The end of the page is aligned by definition to 2^page_order > > * Any primitive types such as integer, etc. are hence aligned > > * Structures are padded so that the total size is multiple of > the largest alignment required (because otherwise arrays of > structures would end up causing unaligned access as well). > > That leaves out things such as buffers for direct I/O, the only way to > allocate aligned buffers there is to make the size to be multiple of > the block size. I don't have concrete example at hand, but I foggily recall s390 issue from couple years back, where it didn't like odd addresses. Can't recall if it was data or code pointer. Could we apply/enforce some minimum alignment, similar to what glibc does for malloc?
Hi! > > I'm aware of that. My reasoning here is that: > > > > * The end of the page is aligned by definition to 2^page_order > > > > * Any primitive types such as integer, etc. are hence aligned > > > > * Structures are padded so that the total size is multiple of > > the largest alignment required (because otherwise arrays of > > structures would end up causing unaligned access as well). > > > > That leaves out things such as buffers for direct I/O, the only way to > > allocate aligned buffers there is to make the size to be multiple of > > the block size. > > I don't have concrete example at hand, but I foggily recall > s390 issue from couple years back, where it didn't like odd addresses. > Can't recall if it was data or code pointer. Data should be fine as far as they are aligned accordingly to the type sizes. I.e. one byte acces is fine on odd addresses, otherwise most of the functions in string.h wouldn't work. For shorts i.e. two byte integers odd addresses are slower on x86 and x86_64 however does not work at all on many architectures. I remember that 32bit arm used to have in-kernel emulation that mostly did the right job but sometimes you got wrong results, so unaligned accesses are better to be avoided. The question is if kernel expects some alignment for buffers for things such as read()/write() etc. I doubt so, since that would still break things like write(fd, "aabbcc" + 1, 3) which I would expect is still valid code. Or do you have anything else in mind that may break? > Could we apply/enforce some minimum alignment, similar to what glibc > does for malloc? That would be against the purpose of this patchset, i.e. catching off-by-one bugs, since the page boundary would be a few bytes after the end of the buffer in some cases. Well I guess that most of the allocations would be as a matter of fact aligned and even these that are not could be easily fixed by choosing buffers that are multiples of four. I would be fine with aligning the buffers for architectures that turn out to be problematic if we find some. However I would like to avoid to "just in case" modifications.
----- Original Message ----- > Hi! > > > I'm aware of that. My reasoning here is that: > > > > > > * The end of the page is aligned by definition to 2^page_order > > > > > > * Any primitive types such as integer, etc. are hence aligned > > > > > > * Structures are padded so that the total size is multiple of > > > the largest alignment required (because otherwise arrays of > > > structures would end up causing unaligned access as well). > > > > > > That leaves out things such as buffers for direct I/O, the only way to > > > allocate aligned buffers there is to make the size to be multiple of > > > the block size. > > > > I don't have concrete example at hand, but I foggily recall > > s390 issue from couple years back, where it didn't like odd addresses. > > Can't recall if it was data or code pointer. > > Data should be fine as far as they are aligned accordingly to the type sizes. > > I.e. one byte acces is fine on odd addresses, otherwise most of the > functions in string.h wouldn't work. > > For shorts i.e. two byte integers odd addresses are slower on x86 and > x86_64 however does not work at all on many architectures. I remember > that 32bit arm used to have in-kernel emulation that mostly did the > right job but sometimes you got wrong results, so unaligned accesses are > better to be avoided. > > The question is if kernel expects some alignment for buffers for things > such as read()/write() etc. I doubt so, since that would still break > things like write(fd, "aabbcc" + 1, 3) which I would expect is still > valid code. > > Or do you have anything else in mind that may break? I was thinking of buffers, but as it turns out the example I had in mind was about alignment of symbols: [ 1.888972] Loading compiled-in X.509 certificates [ 1.888974] Problem parsing in-kernel X.509 certificate list Dump of assembler code for function load_system_certificate_list: 0x00000000009ad2c0 <+0>: stmg %r6,%r15,72(%r15) 0x00000000009ad2c6 <+6>: larl %r13,0x64bdb8 0x00000000009ad2cc <+12>: larl %r2,0x799032 0x00000000009ad2d2 <+18>: tmll %r15,16256 0x00000000009ad2d6 <+22>: lgr %r14,%r15 0x00000000009ad2da <+26>: lay %r15,-104(%r15) 0x00000000009ad2e0 <+32>: je 0x9ad2e2 <load_system_certificate_list+34> 0x00000000009ad2e4 <+36>: stg %r14,152(%r15) 0x00000000009ad2ea <+42>: larl %r8,0x9ee28c <__setup_str_nosmp+5> ^^^^^^^^ From z/Architecture Principles of Operation SA22-7832-07: "Only even addresses (halfword addresses) can be generated. If an odd address is desired, LOAD ADDRESS can be used to add one to an address formed by LOAD ADDRESS RELATIVE LONG." > > > Could we apply/enforce some minimum alignment, similar to what glibc > > does for malloc? > > That would be against the purpose of this patchset, i.e. catching > off-by-one bugs, since the page boundary would be a few bytes after the > end of the buffer in some cases. Well I guess that most of the > allocations would be as a matter of fact aligned and even these that are > not could be easily fixed by choosing buffers that are multiples of > four. OK, that's a fair point. > > I would be fine with aligning the buffers for architectures that turn > out to be problematic if we find some. However I would like to avoid to > "just in case" modifications. > > -- > Cyril Hrubis > chrubis@suse.cz >
On Thu, Aug 1, 2019 at 5:26 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > In a case that the buffer size is not a multiple of a page size there is > unused space before the start of the buffer. Let's fill that with > center mirrored random bytes and check that the buffer wasn't modified > before we unmap it. I like this check canary part. Amazing to me. > + > +static void check_canary(struct map *map) > +{ > + size_t i; > + char *buf = map->addr; > + > + for (i = 0; i < map->buf_shift/2; i++) { > + if (buf[map->buf_shift - i - 1] != buf[i]) { > + tst_res(TWARN, > + "pid %i: buffer modified before address %p %zu", > + (char*)map->addr + map->buf_shift, i); Here you probably missed the getpid() for printing %i match :).
Hi! > > +static void check_canary(struct map *map) > > +{ > > + size_t i; > > + char *buf = map->addr; > > + > > + for (i = 0; i < map->buf_shift/2; i++) { > > + if (buf[map->buf_shift - i - 1] != buf[i]) { > > + tst_res(TWARN, > > + "pid %i: buffer modified before address %p %zu", > > + (char*)map->addr + map->buf_shift, i); > > Here you probably missed the getpid() for printing %i match :). Ah, right, I guess that this is unfinished last minute modification. Actually as it is the canaries are checked only for the main library pid becuase the tst_free_all() is hooked up in the do_test_cleanup(). I guess that we should call it in the run_tests() function before we call exit(0) for the child processes as well as in the tst_vbrk_() and after that changes printing pid in the warning makes sense.
diff --git a/lib/newlib_tests/test_guarded_buf.c b/lib/newlib_tests/test_guarded_buf.c index 2951dce23..e41d3fa99 100644 --- a/lib/newlib_tests/test_guarded_buf.c +++ b/lib/newlib_tests/test_guarded_buf.c @@ -21,9 +21,17 @@ static char *buf3; static void do_test(unsigned int n) { - int pid = SAFE_FORK(); + int pid; int status; + if (n == 6) { + buf1[-1] = 0; + buf3[-1] = 0; + tst_res(TPASS, "Buffers dirtied!"); + return; + } + + pid = SAFE_FORK(); if (!pid) { switch (n) { case 0: @@ -69,7 +77,7 @@ static void do_test(unsigned int n) static struct tst_test test = { .forks_child = 1, .test = do_test, - .tcnt = 6, + .tcnt = 7, .bufs = (struct tst_buffers []) { {&buf1, .size = BUF1_LEN}, {&buf2, .size = BUF2_LEN}, diff --git a/lib/tst_buffers.c b/lib/tst_buffers.c index c4b2859d1..bc0cb50d8 100644 --- a/lib/tst_buffers.c +++ b/lib/tst_buffers.c @@ -11,11 +11,38 @@ struct map { void *addr; size_t size; + size_t buf_shift; struct map *next; }; static struct map *maps; +static void setup_canary(struct map *map) +{ + size_t i; + char *buf = map->addr; + + for (i = 0; i < map->buf_shift/2; i++) { + char c = random(); + buf[map->buf_shift - i - 1] = c; + buf[i] = c; + } +} + +static void check_canary(struct map *map) +{ + size_t i; + char *buf = map->addr; + + for (i = 0; i < map->buf_shift/2; i++) { + if (buf[map->buf_shift - i - 1] != buf[i]) { + tst_res(TWARN, + "pid %i: buffer modified before address %p %zu", + (char*)map->addr + map->buf_shift, i); + } + } +} + void *tst_alloc(size_t size) { size_t page_size = getpagesize(); @@ -34,9 +61,13 @@ void *tst_alloc(size_t size) maps = map; if (size % page_size) - ret += page_size - (size % page_size); + map->buf_shift = page_size - (size % page_size); + else + map->buf_shift = 0; + + setup_canary(map); - return ret; + return ret + map->buf_shift; } static int count_iovec(int *sizes) @@ -97,6 +128,7 @@ void tst_free_all(void) while (i) { struct map *j = i; tst_res(TINFO, "Freeing %p %zu", i->addr, i->size); + check_canary(i); SAFE_MUNMAP(i->addr, i->size); i = i->next; free(j);
In a case that the buffer size is not a multiple of a page size there is unused space before the start of the buffer. Let's fill that with center mirrored random bytes and check that the buffer wasn't modified before we unmap it. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- lib/newlib_tests/test_guarded_buf.c | 12 ++++++++-- lib/tst_buffers.c | 36 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 4 deletions(-)