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, 9 Dec 2013 13:32:09 +0000 Message-ID: <52A5C659.4040902@citrix.com> References: <1385390321-10124-1-git-send-email-andrew.cooper3@citrix.com> <529C88AC.5070706@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <529C88AC.5070706@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 again? On 02/12/13 13:18, Andrew Cooper wrote: > 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