[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 <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.

> > 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;
> > >  
> > >   
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160211/c2d4106f/attachment.sig>