[RFC,2/9] lib: Add a canary for guarded buffers
diff mbox series

Message ID 20190801092616.30553-3-chrubis@suse.cz
State Superseded
Headers show
Series
  • [RFC,1/9] lib: Add support for guarded buffers
Related show

Commit Message

Cyril Hrubis Aug. 1, 2019, 9:26 a.m. UTC
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(-)

Comments

Jan Stancek Aug. 1, 2019, 10:43 a.m. UTC | #1
----- 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.
Cyril Hrubis Aug. 1, 2019, 11:54 a.m. UTC | #2
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.
Jan Stancek Aug. 1, 2019, 4:32 p.m. UTC | #3
----- 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?
Cyril Hrubis Aug. 2, 2019, 9:47 a.m. UTC | #4
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.
Jan Stancek Aug. 2, 2019, 10:54 a.m. UTC | #5
----- 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
>
Li Wang Aug. 3, 2019, 1:02 p.m. UTC | #6
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 :).
Cyril Hrubis Aug. 8, 2019, 9:27 a.m. UTC | #7
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.

Patch
diff mbox series

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);