Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Jonas Ådahl]

On Thu, Feb 11, 2016 at 11:34:15AM +0200, Pekka Paalanen wrote:
> On Thu, 11 Feb 2016 09:47:29 +0800
> Jonas Ådahl  wrote:
> 
> > On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote:
> > > On 10/02/16 09:35 AM, Jonas Ådahl wrote:  
> > > > When we are adding padding bytes making our wl_buffer buffer content 4
> > > > byte aligned, we are just moving the pointer. Since the buffer is
> > > > allocated using plain malloc(), this means our padding bytes are
> > > > effectively uninitialized data, which could be anything previously
> > > > allocated in the server process. As we'll be sharing this buffer
> > > > content with arbitrary clients, we are effectively sharing private
> > > > memory with every client, and even though a well behaving client will
> > > > discard any such memory, a malicious client may not.
> > > > 
> > > > Therefor, to avoid any potential missuse of the uninitialized padding
> > > > memory shared between the server and client, initialize the buffer
> > > > content to 0, making the padding bytes always 0.  
> > > 
> > > Cool - is this the cause of the valgrind complaints I've never bothered
> > > to track down? :)  
> > 
> > Yes. Should have mentioned that in the commit message, as well as the
> > bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me
> > actually dig out what memory was uninitialized. I'll add those two
> > things to the commit message.
> > 
> > >   
> > > > Signed-off-by: Jonas Ådahl 
> > > > ---
> > > >  src/connection.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/connection.c b/src/connection.c
> > > > index 65b64e9..c0e322f 100644
> > > > --- a/src/connection.c
> > > > +++ b/src/connection.c
> > > > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, 
> > > > struct wl_connection *connection)
> > > > return -1;
> > > >  
> > > > buffer_size = buffer_size_for_closure(closure);
> > > > -   buffer = malloc(buffer_size * sizeof buffer[0]);
> > > > +   buffer = zalloc(buffer_size * sizeof buffer[0]);  
> > > 
> > > Oh, took me a couple of reads to realize the padding is between certain
> > > potential members in the closure. (and not trivially at the start or end
> > > of the buffer)
> 
> Hi,
> 
> just to see I understood right too, you mean the string and array types
> in serialize_closure() which use DIV_ROUNDUP() and nothing else?

Yes. An alternative solution I tested just for the sake of it was to
memset the padding up to the next DIV_ROUNDUP(), and that also fixed the
warning, but I as well think a malloc->zalloc was the cleaner approach.


Jonas

> 
> If that is the case, then have my
> Reviewed-by: Pekka Paalanen 
> 
> and with that I also nominate this patch to be landed for 1.10: it is
> trivial and obviously correct, and if it in the unlikely event causes
> any problems, those problems have existed before. It might even be
> considered as a security fix by some far stretch.
> 
> > > I guess we could zero just the pad bytes in serialize_closure, I don't
> > > know whether that falls under "premature optimization" or not, though.
> > > Looks pretty easy to do...  
> > 
> > We could do that, but is calloc() really any noticably slower than
> > malloc() in the way we are using it?
> 
> I wouldn't think there is any observable performance penalty here. And
> I like the simplicity of the patch and the catch-all nature of the fix.
> 
> 
> Thanks,
> pq
> 
> > It is fairly easy (I did it at first just to make sure it was those
> > bytes only that triggered the warning), but s/malloc/zalloc/ seemed like
> > the cleaner solution to me.
> > 
> > 
> > Jonas
> > 
> > > 
> > > In any case,
> > > Reviewed-by: Derek Foreman 
> > > 
> > > Thanks,
> > > Derek
> > >   
> > > > if (buffer == NULL)
> > > > return -1;
> > > >  
> > > >   


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Pekka Paalanen]

On Thu, 11 Feb 2016 09:47:29 +0800
Jonas Ådahl  wrote:

> On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote:
> > On 10/02/16 09:35 AM, Jonas Ådahl wrote:  
> > > When we are adding padding bytes making our wl_buffer buffer content 4
> > > byte aligned, we are just moving the pointer. Since the buffer is
> > > allocated using plain malloc(), this means our padding bytes are
> > > effectively uninitialized data, which could be anything previously
> > > allocated in the server process. As we'll be sharing this buffer
> > > content with arbitrary clients, we are effectively sharing private
> > > memory with every client, and even though a well behaving client will
> > > discard any such memory, a malicious client may not.
> > > 
> > > Therefor, to avoid any potential missuse of the uninitialized padding
> > > memory shared between the server and client, initialize the buffer
> > > content to 0, making the padding bytes always 0.  
> > 
> > Cool - is this the cause of the valgrind complaints I've never bothered
> > to track down? :)  
> 
> Yes. Should have mentioned that in the commit message, as well as the
> bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me
> actually dig out what memory was uninitialized. I'll add those two
> things to the commit message.
> 
> >   
> > > Signed-off-by: Jonas Ådahl 
> > > ---
> > >  src/connection.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/connection.c b/src/connection.c
> > > index 65b64e9..c0e322f 100644
> > > --- a/src/connection.c
> > > +++ b/src/connection.c
> > > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, struct 
> > > wl_connection *connection)
> > >   return -1;
> > >  
> > >   buffer_size = buffer_size_for_closure(closure);
> > > - buffer = malloc(buffer_size * sizeof buffer[0]);
> > > + buffer = zalloc(buffer_size * sizeof buffer[0]);  
> > 
> > Oh, took me a couple of reads to realize the padding is between certain
> > potential members in the closure. (and not trivially at the start or end
> > of the buffer)

Hi,

just to see I understood right too, you mean the string and array types
in serialize_closure() which use DIV_ROUNDUP() and nothing else?

If that is the case, then have my
Reviewed-by: Pekka Paalanen 

and with that I also nominate this patch to be landed for 1.10: it is
trivial and obviously correct, and if it in the unlikely event causes
any problems, those problems have existed before. It might even be
considered as a security fix by some far stretch.

> > I guess we could zero just the pad bytes in serialize_closure, I don't
> > know whether that falls under "premature optimization" or not, though.
> > Looks pretty easy to do...  
> 
> We could do that, but is calloc() really any noticably slower than
> malloc() in the way we are using it?

I wouldn't think there is any observable performance penalty here. And
I like the simplicity of the patch and the catch-all nature of the fix.


Thanks,
pq

> It is fairly easy (I did it at first just to make sure it was those
> bytes only that triggered the warning), but s/malloc/zalloc/ seemed like
> the cleaner solution to me.
> 
> 
> Jonas
> 
> > 
> > In any case,
> > Reviewed-by: Derek Foreman 
> > 
> > Thanks,
> > Derek
> >   
> > >   if (buffer == NULL)
> > >   return -1;
> > >  
> > >   


pgpnMOJKjPqKr.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Bryce Harrington]

On Thu, Feb 11, 2016 at 11:34:15AM +0200, Pekka Paalanen wrote:
> On Thu, 11 Feb 2016 09:47:29 +0800
> Jonas Ådahl  wrote:
> 
> > On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote:
> > > On 10/02/16 09:35 AM, Jonas Ådahl wrote:  
> > > > When we are adding padding bytes making our wl_buffer buffer content 4
> > > > byte aligned, we are just moving the pointer. Since the buffer is
> > > > allocated using plain malloc(), this means our padding bytes are
> > > > effectively uninitialized data, which could be anything previously
> > > > allocated in the server process. As we'll be sharing this buffer
> > > > content with arbitrary clients, we are effectively sharing private
> > > > memory with every client, and even though a well behaving client will
> > > > discard any such memory, a malicious client may not.
> > > > 
> > > > Therefor, to avoid any potential missuse of the uninitialized padding
> > > > memory shared between the server and client, initialize the buffer
> > > > content to 0, making the padding bytes always 0.  
> > > 
> > > Cool - is this the cause of the valgrind complaints I've never bothered
> > > to track down? :)  
> > 
> > Yes. Should have mentioned that in the commit message, as well as the
> > bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me
> > actually dig out what memory was uninitialized. I'll add those two
> > things to the commit message.
> > 
> > >   
> > > > Signed-off-by: Jonas Ådahl 
> > > > ---
> > > >  src/connection.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/connection.c b/src/connection.c
> > > > index 65b64e9..c0e322f 100644
> > > > --- a/src/connection.c
> > > > +++ b/src/connection.c
> > > > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, 
> > > > struct wl_connection *connection)
> > > > return -1;
> > > >  
> > > > buffer_size = buffer_size_for_closure(closure);
> > > > -   buffer = malloc(buffer_size * sizeof buffer[0]);
> > > > +   buffer = zalloc(buffer_size * sizeof buffer[0]);  
> > > 
> > > Oh, took me a couple of reads to realize the padding is between certain
> > > potential members in the closure. (and not trivially at the start or end
> > > of the buffer)
> 
> Hi,
> 
> just to see I understood right too, you mean the string and array types
> in serialize_closure() which use DIV_ROUNDUP() and nothing else?
> 
> If that is the case, then have my
> Reviewed-by: Pekka Paalanen 
> 
> and with that I also nominate this patch to be landed for 1.10: it is
> trivial and obviously correct, and if it in the unlikely event causes
> any problems, those problems have existed before. It might even be
> considered as a security fix by some far stretch.

Yep, good simple fix:

Reviewed-by: Bryce Harrington 

and landed for 1.10:
To ssh://git.freedesktop.org/git/wayland/wayland
   1906a90..bf34ac7  master -> master

Bryce
 
> > > I guess we could zero just the pad bytes in serialize_closure, I don't
> > > know whether that falls under "premature optimization" or not, though.
> > > Looks pretty easy to do...  
> > 
> > We could do that, but is calloc() really any noticably slower than
> > malloc() in the way we are using it?
> 
> I wouldn't think there is any observable performance penalty here. And
> I like the simplicity of the patch and the catch-all nature of the fix.
>
> Thanks,
> pq
> 
> > It is fairly easy (I did it at first just to make sure it was those
> > bytes only that triggered the warning), but s/malloc/zalloc/ seemed like
> > the cleaner solution to me.
> > 
> > 
> > Jonas
> > 
> > > 
> > > In any case,
> > > Reviewed-by: Derek Foreman 
> > > 
> > > Thanks,
> > > Derek
> > >   
> > > > if (buffer == NULL)
> > > > return -1;
> > > >  
> > > >   

> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2
> 
> iQIVAwUBVrxVlyNf5bQRqqqnAQgpyhAAkBMiboYyTiE7MRPuNc3LfDxnfx9NXJdB
> SY4miIA4VUu4XsZWSaXE6/48HFnFqlDaNhKuPpHCap8aInMrG0T1ZDw2McYGLM8Y
> g3U48ZyQNvzyHE+PzOWcdPfmGsKGkwsmA+Epo16DEAqbbzWV+pu66Ny0gILWHPmX
> g7i206GJ3eetYdQPtj0HdxPZe7zFTNMI9dDLhEFPNFDwSm+I+RdJICXoMCFYyL7z
> OSXj0X7OWAJk0/oFqfV3DH2xXi6elW629GJmUgNVfF5GqP6sSzaGerccr6sYjXI5
> kTyL6CP1C6kNt+ylL0FG/80Oy3ET7/7+FnUQ/auic9UWGAwRz5hbpdj9rKcmpqf7
> zcz2SmGPGH74O/ZFK5NEkfU9TkDrwFUt/jtZ9Bwak4WyrmG0XhyuTz3vvP6fEBsj
> dCzZy/W9LE8Fj5Yp1QM9tLsaITpgMjeHyE3kdcOBMq1Dg5rcKsTCn++YarQ7NY6X
> zMF9Dw469GHmJP1+HdDxmDvH5tdWPm0nCXozx/7sYS/So4DElPHALj0QkY0tNI7U
> X47SxYrBklJIglxvwdnh8MfmBQN8n5gMK3KjIxHST+iIgK0W9jUnjma5XoKXdpVm
> SmK3AsJmCegyXmpUsQjCnS+wlI8Lv6NLI2chxpztVL1uBLKN69Bs7155/LLtQE8x
> BX38/Od7YBc=
> =YXcL
> -END PGP SIGNATURE-

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Jonas Ådahl]

When we are adding padding bytes making our wl_buffer buffer content 4
byte aligned, we are just moving the pointer. Since the buffer is
allocated using plain malloc(), this means our padding bytes are
effectively uninitialized data, which could be anything previously
allocated in the server process. As we'll be sharing this buffer
content with arbitrary clients, we are effectively sharing private
memory with every client, and even though a well behaving client will
discard any such memory, a malicious client may not.

Therefor, to avoid any potential missuse of the uninitialized padding
memory shared between the server and client, initialize the buffer
content to 0, making the padding bytes always 0.

Signed-off-by: Jonas Ådahl 
---
 src/connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/connection.c b/src/connection.c
index 65b64e9..c0e322f 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, struct 
wl_connection *connection)
return -1;
 
buffer_size = buffer_size_for_closure(closure);
-   buffer = malloc(buffer_size * sizeof buffer[0]);
+   buffer = zalloc(buffer_size * sizeof buffer[0]);
if (buffer == NULL)
return -1;
 
-- 
2.4.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Derek Foreman]

On 10/02/16 09:35 AM, Jonas Ådahl wrote:
> When we are adding padding bytes making our wl_buffer buffer content 4
> byte aligned, we are just moving the pointer. Since the buffer is
> allocated using plain malloc(), this means our padding bytes are
> effectively uninitialized data, which could be anything previously
> allocated in the server process. As we'll be sharing this buffer
> content with arbitrary clients, we are effectively sharing private
> memory with every client, and even though a well behaving client will
> discard any such memory, a malicious client may not.
> 
> Therefor, to avoid any potential missuse of the uninitialized padding
> memory shared between the server and client, initialize the buffer
> content to 0, making the padding bytes always 0.

Cool - is this the cause of the valgrind complaints I've never bothered
to track down? :)

> Signed-off-by: Jonas Ådahl 
> ---
>  src/connection.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/connection.c b/src/connection.c
> index 65b64e9..c0e322f 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, struct 
> wl_connection *connection)
>   return -1;
>  
>   buffer_size = buffer_size_for_closure(closure);
> - buffer = malloc(buffer_size * sizeof buffer[0]);
> + buffer = zalloc(buffer_size * sizeof buffer[0]);

Oh, took me a couple of reads to realize the padding is between certain
potential members in the closure. (and not trivially at the start or end
of the buffer)

I guess we could zero just the pad bytes in serialize_closure, I don't
know whether that falls under "premature optimization" or not, though.
Looks pretty easy to do...

In any case,
Reviewed-by: Derek Foreman 

Thanks,
Derek

>   if (buffer == NULL)
>   return -1;
>  
> 

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Jonas Ådahl]

On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote:
> On 10/02/16 09:35 AM, Jonas Ådahl wrote:
> > When we are adding padding bytes making our wl_buffer buffer content 4
> > byte aligned, we are just moving the pointer. Since the buffer is
> > allocated using plain malloc(), this means our padding bytes are
> > effectively uninitialized data, which could be anything previously
> > allocated in the server process. As we'll be sharing this buffer
> > content with arbitrary clients, we are effectively sharing private
> > memory with every client, and even though a well behaving client will
> > discard any such memory, a malicious client may not.
> > 
> > Therefor, to avoid any potential missuse of the uninitialized padding
> > memory shared between the server and client, initialize the buffer
> > content to 0, making the padding bytes always 0.
> 
> Cool - is this the cause of the valgrind complaints I've never bothered
> to track down? :)

Yes. Should have mentioned that in the commit message, as well as the
bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me
actually dig out what memory was uninitialized. I'll add those two
things to the commit message.

> 
> > Signed-off-by: Jonas Ådahl 
> > ---
> >  src/connection.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/connection.c b/src/connection.c
> > index 65b64e9..c0e322f 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, struct 
> > wl_connection *connection)
> > return -1;
> >  
> > buffer_size = buffer_size_for_closure(closure);
> > -   buffer = malloc(buffer_size * sizeof buffer[0]);
> > +   buffer = zalloc(buffer_size * sizeof buffer[0]);
> 
> Oh, took me a couple of reads to realize the padding is between certain
> potential members in the closure. (and not trivially at the start or end
> of the buffer)
> 
> I guess we could zero just the pad bytes in serialize_closure, I don't
> know whether that falls under "premature optimization" or not, though.
> Looks pretty easy to do...

We could do that, but is calloc() really any noticably slower than
malloc() in the way we are using it?

It is fairly easy (I did it at first just to make sure it was those
bytes only that triggered the warning), but s/malloc/zalloc/ seemed like
the cleaner solution to me.


Jonas

> 
> In any case,
> Reviewed-by: Derek Foreman 
> 
> Thanks,
> Derek
> 
> > if (buffer == NULL)
> > return -1;
> >  
> > 
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel