From patchwork Wed Apr 18 04:08:47 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 153376 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 03FB2B6EEB for ; Wed, 18 Apr 2012 14:08:51 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750948Ab2DREIt (ORCPT ); Wed, 18 Apr 2012 00:08:49 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:54510 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750769Ab2DREIr convert rfc822-to-8bit (ORCPT ); Wed, 18 Apr 2012 00:08:47 -0400 Received: by pbcun15 with SMTP id un15so8430945pbc.19 for ; Tue, 17 Apr 2012 21:08:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=lCQROigAK8TtzGKB4p0Xpe4CsfreK/w/yb/FxR8Np9c=; b=X8/P7jx2YrfDn5ZIXbMhODEdYpc2EKnhv+hgaIc+OODJKISTK3BjkMgpoC/W7vcjrO b9QVyxwwfY5aswCxTGCYcve0/xkKkUT7f6DQIqchF0ecqM2Qn6Q/LHcGhOnCdD93eNLL kft2AZK0D9FOFjSGR4+nsobF5QJzY30HsMFjlGZoEuDmwV5B5k1wd8yXWrv77AN81Ej0 DefPVBVDUw2BnAPo7XlCcE2eW9dhgLtcSXI+gfCn8xuUTHIWNXrDKGhDoaY4F4eBCQiy CDTqU5DZKnpKOhTxluM5TV9vZGOiU6NqfEck2vNRx5H2QKsNSrgsf8HgEcOwMOXOMGE7 +5Sg== MIME-Version: 1.0 Received: by 10.68.233.99 with SMTP id tv3mr3022487pbc.8.1334722127102; Tue, 17 Apr 2012 21:08:47 -0700 (PDT) Received: by 10.68.50.100 with HTTP; Tue, 17 Apr 2012 21:08:47 -0700 (PDT) In-Reply-To: <20120417.223614.629911246108750471.davem@davemloft.net> References: <4F8D497F.8060601@gmail.com> <20120417.223614.629911246108750471.davem@davemloft.net> Date: Wed, 18 Apr 2012 00:08:47 -0400 X-Google-Sender-Auth: gtFiFZncQGcqq9HHfmMdh7K85BU Message-ID: Subject: Re: [patch] Fix handling of overlength pathname in AF_UNIX sun_path From: "Carlos O'Donell" To: David Miller Cc: mtk.manpages@gmail.com, netdev@vger.kernel.org, penguin-kernel@i-love.sakura.ne.jp, linux-api@vger.kernel.org, yoshfuji@linux-ipv6.org, jengelh@medozas.de, w@1wt.eu, alan@lxorguk.ukuu.org.uk Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, Apr 17, 2012 at 10:36 PM, David Miller wrote: > From: Michael Kerrisk > Date: Tue, 17 Apr 2012 22:44:15 +1200 > >> 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. > > The problem with this logic is that it ignores every single Linux > system that exists right now. > > You need to code this logic into your application unless you don't > want it to run properly on every Linux system that actually exists. > > Sorry, we're not making this change. Dave, I don't clearly understand your position here, and perhaps that's my own ignorance, but could you please clarify, with examples, exactly why the change is not acceptable? I can see several valid arguments against the change, but I don't know which argument your position asserts. One might assert that careless userspace applications exist that pass `sizeof(struct sockaddr_un)' (or worse) as the 3rd argument to bind instead of SUN_LEN(my_sock). The logic in the patch doesn't account for this, and can't really, and would therefore unacceptably break existing applications by trying to assert the location of a \0 where one doesn't exist. The kernel must therefore continue to null-terminate at the specified length, possibly the 109th character, and use strlen to capture the true length of the path. The kernel knows that in the worst case a non-null terminated path might contain some garbage, but that's the users fault, and the logic to prevent this must exist in the application not the kernel. The counter argument to all of this is that it's a QoI issue, and that the kernel shouldn't accept accidentally non-null terminated paths, and should instead return EINVAL for them. Not to mention that it's difficult for userspace to easily catch this error in glibc which would need to inspect the sockaddr, duplicating kernel code. Why is it valid for the user path to have no null terminator? Why not have: --- Does that make sense? Cheers, Carlos. -- 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 diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index d510353..f9f77a7 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -216,6 +216,9 @@ static int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned *hashp) */ ((char *)sunaddr)[len] = 0; len = strlen(sunaddr->sun_path)+1+sizeof(short); + /* No null terminator was found in the path. */ + if (len > sizeof(*sunaddr)) + return -EINVAL; return len; }