From patchwork Tue Apr 17 10:44:15 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael Kerrisk \\(man-pages\\)" X-Patchwork-Id: 153122 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id E5703B7008 for ; Tue, 17 Apr 2012 20:44:31 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753794Ab2DQKo3 (ORCPT ); Tue, 17 Apr 2012 06:44:29 -0400 Received: from mail-pz0-f52.google.com ([209.85.210.52]:39425 "EHLO mail-pz0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753613Ab2DQKo2 (ORCPT ); Tue, 17 Apr 2012 06:44:28 -0400 Received: by dake40 with SMTP id e40so8220913dak.11 for ; Tue, 17 Apr 2012 03:44:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :content-type:content-transfer-encoding; bh=WIHidAXE3sJI041y9+cwVR/HUCjM3aPR8jlCTfPuRWw=; b=yqhGn/M2B0zuDfM7iTvb6yCGBJZg8SFqIPVXQobQEphmK7xk7uDiapV7VofXt8Ol/9 Qs/+1v3YqHcW95/a2JebA3mqqKkgrSA7Kwz6Kgd4z4sw5KXQGMCxVow3XX4KZWd/Qyfc 8xkadxWD0CazymiZzP18ZDjqn6dbcrKAJKPeqojhjvluu3ag+AIs7ygzuM0d7+YbG/zW 7lW5oVw/5qsS79HJRpPgdKTbFITgTrgtD16D09ZsZbul1+tJqVeEaF2ElPTICj7a/rrE wZX2P70hdDwPimnn+8FieKQF1K4MEYYjuaI4gl+PXLdMica225Wa9P9zt3x24c/xiMNH Vzgw== Received: by 10.68.233.234 with SMTP id tz10mr36128405pbc.68.1334659467619; Tue, 17 Apr 2012 03:44:27 -0700 (PDT) Received: from [192.168.0.10] (134.250.69.111.dynamic.snap.net.nz. [111.69.250.134]) by mx.google.com with ESMTPS id it8sm4278331pbc.56.2012.04.17.03.44.23 (version=SSLv3 cipher=OTHER); Tue, 17 Apr 2012 03:44:26 -0700 (PDT) Message-ID: <4F8D497F.8060601@gmail.com> Date: Tue, 17 Apr 2012 22:44:15 +1200 From: Michael Kerrisk User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: netdev CC: Tetsuo Handa , linux-api@vger.kernel.org, yoshfuji@linux-ipv6.org, David Miller , Jan Engelhardt , Willy Tarreau , Alan Cox Subject: [patch] Fix handling of overlength pathname in AF_UNIX sun_path Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Tetsuo Handa sent me a patch to document the kernel's odd behavior when asked to create a UNIX domain socket address where the pathname completely fills the sun_path field without including a null terminator [1]. One of the consequences of the current kernel behavior is that when a socket address is returned to userspace (via getsockname(), getpeername(), accept(), recvfrom()), applications can't reliably do things such as: printf("%s\n", addr.sun_path); Instead one must either write: printf("%.*s\n", sizeof(addr.sun_path), addr.sun_path); or, when retrieving a socket address structure, employ a buffer whose size is: sizeof(struct sockaddr_un) + 1 (This ensures that there is enough space to hold the null terminator for the case where a socket was bound to a sun_path with non-NUL characters in all 108 bytes. But it entails some casting.) Tetsuo initially considered there might be a kernel bug here, but an attempt to change the kernel behavior met resistance [2]. The patch at the end of this message is a slightly different fix for the same problem. There are a few reasons why I think it (or some variation) should be considered: 1. Changing the kernel behavior prevents userspace having to go through the sort of contortions described above in order to handle the 108-non-NUL-bytes-in-sun_path case. 2. POSIX says that sun_path is a pathname. By (POSIX) definition, a pathname is null terminated. 3. Considering these two sets: (a) [applications that rely on the assumption that there is a null terminator inside sizeof(sun_path) bytes] (b) [applications that would break if the kernel behavior changed] I suspect that set (a) is rather larger than set (b)--or, more likely still, applications ensure they go for the lowest common denominator limit of 92 (HP-UX) or 104 (historical BSD limit) bytes, and so avoid this issue completely. The accompanying patch changes unix_mkname() to ensure that a terminating null byte is always located within the first 108 bytes of sun_path. It does change the ABI for the former case where a pathname ran to 108 bytes without a null terminator: for that case, the call now fails with the error -EINVAL. What are people's thoughts on applying this? [1] http://thread.gmane.org/gmane.linux.network/174473/focus=1861 [2] http://thread.gmane.org/gmane.linux.kernel/291038 Signed-off-by: Michael Kerrisk --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- net/unix/af_unix.c.orig 2012-04-17 09:50:07.383459311 +1200 +++ net/unix/af_unix.c 2012-04-17 19:49:56.077852639 +1200 @@ -207,14 +207,16 @@ static int unix_mkname(struct sockaddr_u if (!sunaddr || sunaddr->sun_family != AF_UNIX) return -EINVAL; if (sunaddr->sun_path[0]) { - /* - * This may look like an off by one error but it is a bit more - * subtle. 108 is the longest valid AF_UNIX path for a binding. - * sun_path[108] doesn't as such exist. However in kernel space - * we are guaranteed that it is a valid memory location in our - * kernel address buffer. - */ - ((char *)sunaddr)[len] = 0; + if (len == sizeof(*sunaddr)) { + /* + * If 'sun_path' is completely filled, the user + * must include a terminator + */ + if (!memchr(sunaddr->sun_path, '\0', + sizeof(sunaddr->sun_path))) + return -EINVAL; + } else + ((char *)sunaddr)[len] = 0; len = strlen(sunaddr->sun_path)+1+sizeof(short); return len; }