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

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