[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 <jadahl at gmail.com> 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 <jadahl at gmail.com>
> > > > ---
> > > >  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 <pekka.paalanen at collabora.co.uk>
> 
> 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 <bryce at osg.samsung.com>

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 <derekf at osg.samsung.com>
> > > 
> > > 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-----