From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets Date: Mon, 2 Dec 2013 13:18:36 +0000 Message-ID: <529C88AC.5070706@citrix.com> References: <1385390321-10124-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1385390321-10124-1-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Matthew Daley , Ian Jackson , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org Ping? This v2 patch appears to have slipped through the cracks from my set of Coverity fixes. ~Andrew On 25/11/13 14:38, Andrew Cooper wrote: > Coverity ID: 1055996 1056002 > > Cache the xs_daemon_socket{,_ro}() strings to save pointlessly > re-snprintf()'ing the same path, and add explicit size checks against > addr.sun_path before strcpy()'ing into it. > > Signed-off-by: Andrew Cooper > CC: Ian Campbell > CC: Ian Jackson > CC: Matthew Daley > > --- > Changes in v2: > * Use logic similar to f220279c14 > --- > tools/xenstore/xenstored_core.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index ccfdaa3..2324e53 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock) > { > struct sockaddr_un addr; > int *sock, *ro_sock; > + const char *soc_str = xs_daemon_socket(); > + const char *soc_str_ro = xs_daemon_socket_ro(); > + > /* Create sockets for them to listen to. */ > *psock = sock = talloc(talloc_autofree_context(), int); > *sock = socket(PF_UNIX, SOCK_STREAM, 0); > @@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int **pro_sock) > talloc_set_destructor(ro_sock, destroy_fd); > > /* FIXME: Be more sophisticated, don't mug running daemon. */ > - unlink(xs_daemon_socket()); > - unlink(xs_daemon_socket_ro()); > + unlink(soc_str); > + unlink(soc_str_ro); > > addr.sun_family = AF_UNIX; > - strcpy(addr.sun_path, xs_daemon_socket()); > + > + if(strlen(soc_str) >= sizeof(addr.sun_path)) > + barf_perror("socket string '%s' too long", soc_str); > + strcpy(addr.sun_path, soc_str); > if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0) > - barf_perror("Could not bind socket to %s", xs_daemon_socket()); > - strcpy(addr.sun_path, xs_daemon_socket_ro()); > + barf_perror("Could not bind socket to %s", soc_str); > + > + if(strlen(soc_str_ro) >= sizeof(addr.sun_path)) > + barf_perror("socket string '%s' too long", soc_str_ro); > + strcpy(addr.sun_path, soc_str_ro); > if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0) > - barf_perror("Could not bind socket to %s", > - xs_daemon_socket_ro()); > - if (chmod(xs_daemon_socket(), 0600) != 0 > - || chmod(xs_daemon_socket_ro(), 0660) != 0) > + barf_perror("Could not bind socket to %s", soc_str_ro); > + > + if (chmod(soc_str, 0600) != 0 > + || chmod(soc_str_ro, 0660) != 0) > barf_perror("Could not chmod sockets"); > > if (listen(*sock, 1) != 0