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: Fri, 13 Dec 2013 18:13:24 +0000 Message-ID: <52AB4E44.9060309@citrix.com> References: <1385390321-10124-1-git-send-email-andrew.cooper3@citrix.com> <529C88AC.5070706@citrix.com> <52A5C659.4040902@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52A5C659.4040902@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 Given the spirit today of missed pings on patches, Third time lucky? ~Andrew On 09/12/2013 13:32, Andrew Cooper wrote: > 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