Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp1362028ybg; Thu, 11 Jun 2020 07:58:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxWJ4HWdA0Nq+YCu4JCsSiAnvNAyVIW0/AJxCtRzZqtQxTVZlT7okrXXhUy/rtentE7KKjf X-Received: by 2002:a17:906:4b50:: with SMTP id j16mr9254463ejv.415.1591887513595; Thu, 11 Jun 2020 07:58:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591887513; cv=none; d=google.com; s=arc-20160816; b=ohc/gBu7BDBgJFLe7tO4wyuCEqjJao2YpWWDcqIJVRmKXcHhetgslId1uTDF7JGt73 tKq+Q17eknB8s7/W1xREjMZOu+unUKfmAZHmoEYA84SfvhIG4YSiBeSibTC2RDMdT+lG n2Ga+YX7932bTE22rmTtmpAL2hk1ia+Vi3nHgl3SJZyvNtmBqYnXPG3a4ZU8BLNVKIkW nm2G6r3t0X6iuLdDAqB3liFOXoiVhXkFi86ZJ+0qy+RrZ/wjwAzP5I3IqaocUom/0gjN P3ltlK+pGs7pb1g6F6npKYN3zKYmlRlo0m1dHWRCuDbo+YKxHcHbDtXGIYofXkr1eHw8 mdig== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :content-language:accept-language:in-reply-to:references:message-id :date:thread-index:thread-topic:subject:cc:to:from; bh=vRNv3YiFqKadoGdAHmmPr2cACJTqpoTtf3uyETQIC9g=; b=QesEkcJqYB3J723VWC5VjMSmS5K6Yz7Hq9msWZtCFSPa9DdUrV7k+RRL4mZfRtyEeg DfY7WXKopao2zBlEfhDdmwlCm7djqEnTWX35njfPChrsGJ5h2fGiB8gvko9viQaiSEvh R2Vr0+7/2ce/FnNfJjup9bfRo95cJOBfRO4GJnkuuivHP19HpobQhY8ReKMZVh9yT0ol z2iVLCM+wicw2mMu5x1/syWZj4cTjsbzEdES/iF+Qr3tp9PY0o5wloktSBuDTnM7XZt3 UxAbggBo0C35YJOWdKRIWkZZ3fQrzSDBdhrMOlPR1HIifZwi9ixLBKLPRtWxDTof5Q1w 3FZA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aculab.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id bu16si1717492edb.146.2020.06.11.07.58.10; Thu, 11 Jun 2020 07:58:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aculab.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728363AbgFKO42 convert rfc822-to-8bit (ORCPT + 99 others); Thu, 11 Jun 2020 10:56:28 -0400 Received: from eu-smtp-delivery-151.mimecast.com ([207.82.80.151]:60292 "EHLO eu-smtp-delivery-151.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728282AbgFKO41 (ORCPT ); Thu, 11 Jun 2020 10:56:27 -0400 Received: from AcuMS.aculab.com (156.67.243.126 [156.67.243.126]) (Using TLS) by relay.mimecast.com with ESMTP id uk-mta-232-tcofI8yIPCqzxiP3BiEhdQ-1; Thu, 11 Jun 2020 15:56:23 +0100 X-MC-Unique: tcofI8yIPCqzxiP3BiEhdQ-1 Received: from AcuMS.Aculab.com (fd9f:af1c:a25b:0:43c:695e:880f:8750) by AcuMS.aculab.com (fd9f:af1c:a25b:0:43c:695e:880f:8750) with Microsoft SMTP Server (TLS) id 15.0.1347.2; Thu, 11 Jun 2020 15:56:22 +0100 Received: from AcuMS.Aculab.com ([fe80::43c:695e:880f:8750]) by AcuMS.aculab.com ([fe80::43c:695e:880f:8750%12]) with mapi id 15.00.1347.000; Thu, 11 Jun 2020 15:56:22 +0100 From: David Laight To: 'Sargun Dhillon' , Christian Brauner CC: Kees Cook , "containers@lists.linux-foundation.org" , Giuseppe Scrivano , Robert Sesek , Chris Palmer , Jann Horn , Greg Kroah-Hartman , Daniel Wagner , "linux-kernel@vger.kernel.org" , Matt Denton , John Fastabend , "linux-fsdevel@vger.kernel.org" , Tejun Heo , Al Viro , "cgroups@vger.kernel.org" , "stable@vger.kernel.org" , "David S . Miller" Subject: RE: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Thread-Topic: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes Thread-Index: AQHWP+BcCi14oegu0U6J73sUpcDiU6jTfHDA Date: Thu, 11 Jun 2020 14:56:22 +0000 Message-ID: <067f494d55c14753a31657f958cb0a6e@AcuMS.aculab.com> References: <20200604125226.eztfrpvvuji7cbb2@wittgenstein> <20200605075435.GA3345@ircssh-2.c.rugged-nimbus-611.internal> <202006091235.930519F5B@keescook> <20200609200346.3fthqgfyw3bxat6l@wittgenstein> <202006091346.66B79E07@keescook> <037A305F-B3F8-4CFA-B9F8-CD4C9EF9090B@ubuntu.com> <202006092227.D2D0E1F8F@keescook> <20200610081237.GA23425@ircssh-2.c.rugged-nimbus-611.internal> <202006101953.899EFB53@keescook> <20200611100114.awdjswsd7fdm2uzr@wittgenstein> <20200611110630.GB30103@ircssh-2.c.rugged-nimbus-611.internal> In-Reply-To: <20200611110630.GB30103@ircssh-2.c.rugged-nimbus-611.internal> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Sargun Dhillon > Sent: 11 June 2020 12:07 > Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes > > On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote: > > On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote: > > > On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote: > > > > As an aside, all of this junk should be dropped: > > > > + ret = get_user(size, &uaddfd->size); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); > > > > + if (ret) > > > > + return ret; > > > > > > > > and the size member of the seccomp_notif_addfd struct. I brought this up > > > > off-list with Tycho that ioctls have the size of the struct embedded in them. We > > > > should just use that. The ioctl definition is based on this[2]: > > > > #define _IOC(dir,type,nr,size) \ > > > > (((dir) << _IOC_DIRSHIFT) | \ > > > > ((type) << _IOC_TYPESHIFT) | \ > > > > ((nr) << _IOC_NRSHIFT) | \ > > > > ((size) << _IOC_SIZESHIFT)) > > > > > > > > > > > > We should just use copy_from_user for now. In the future, we can either > > > > introduce new ioctl names for new structs, or extract the size dynamically from > > > > the ioctl (and mask it out on the switch statement in seccomp_notify_ioctl. > > > > > > Yeah, that seems reasonable. Here's the diff for that part: > > > > Why does it matter that the ioctl() has the size of the struct embedded > > within? Afaik, the kernel itself doesn't do anything with that size. It > > merely checks that the size is not pathological and it does so at > > compile time. > > > > #ifdef __CHECKER__ > > #define _IOC_TYPECHECK(t) (sizeof(t)) > > #else > > /* provoke compile error for invalid uses of size argument */ > > extern unsigned int __invalid_size_argument_for_IOC; > > #define _IOC_TYPECHECK(t) \ > > ((sizeof(t) == sizeof(t[1]) && \ > > sizeof(t) < (1 << _IOC_SIZEBITS)) ? \ > > sizeof(t) : __invalid_size_argument_for_IOC) > > #endif > > > > The size itself is not verified at runtime. copy_struct_from_user() > > still makes sense at least if we're going to allow expanding the struct > > in the future. > Right, but if we simply change our headers and extend the struct, it will break > all existing programs compiled against those headers. In order to avoid that, if > we intend on extending this struct by appending to it, we need to have a > backwards compatibility mechanism. Just having copy_struct_from_user isn't > enough. The data structure either must be fixed size, or we need a way to handle > multiple ioctl numbers derived from headers with different sized struct arguments > > The two approaches I see are: > 1. use more indirection. This has previous art in drm[1]. That's look > something like this: > > struct seccomp_notif_addfd_ptr { > __u64 size; > __u64 addr; > } > > ... And then it'd be up to us to dereference the addr and copy struct from user. Do not go down that route. It isn't worth the pain. You should also assume that userspace might have a compile-time check on the buffer length (I've written one - not hard) and that the kernel might (in the future - or on a BSD kernel) be doing the user copies for you. Also, if you change the structure you almost certainly need to change the name of the ioctl cmd as well as its value. Otherwise a recompiled program will pass the new cmd value (and hopefully the right sized buffer) but it won't have initialised the buffer properly. This is likely to lead to unexpected behaviour. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)