From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D195EC2D0EF for ; Fri, 17 Apr 2020 19:50:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8376820715 for ; Fri, 17 Apr 2020 19:50:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="MHwDf9SX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8376820715 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 1FE818E0003; Fri, 17 Apr 2020 15:50:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1D5728E0001; Fri, 17 Apr 2020 15:50:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0EB6C8E0003; Fri, 17 Apr 2020 15:50:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0097.hostedemail.com [216.40.44.97]) by kanga.kvack.org (Postfix) with ESMTP id E97DF8E0001 for ; Fri, 17 Apr 2020 15:50:25 -0400 (EDT) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 9DA95181AEF30 for ; Fri, 17 Apr 2020 19:50:25 +0000 (UTC) X-FDA: 76718388810.29.spy53_3aee87c9ed28 X-HE-Tag: spy53_3aee87c9ed28 X-Filterd-Recvd-Size: 6122 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf21.hostedemail.com (Postfix) with ESMTP for ; Fri, 17 Apr 2020 19:50:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=oZFKaAEx7x9RM0yplSN83xASDmB3ff/b3yogXeNlHSg=; b=MHwDf9SXtGhOxDOC9x1npNeXsu dLvd8csRf6Q/wv1NcXob0gmK6bvBDyGUpM8ox6iV59gyXMbwRY6oI/dse4DNj7svbiPiGjz8KxWp3 CmSDdKo+QA+LAOHozOfSCUOoQ3cEHroZsR1k4tAtpc3ZAciJSxF6NSGzvl9hENveamfdch5oSSdT4 q4IniJbR9Cdze+AE4xNE2Td8S6Ug7viKtRySWJKbtp9IdTXYOPQ2uwVAZymcePdNcTbGv7UqPCOPB Q6I5e8bI6GEtYRzpLT5qa6uBCrCu2I7OuTTlWhi2+rbXJdAx+sGsWGweKO0WeZSbxuhumFeWWwtTw 5f9T8OtQ==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1jPX0B-0007D5-JD; Fri, 17 Apr 2020 19:50:15 +0000 Date: Fri, 17 Apr 2020 12:50:15 -0700 From: Matthew Wilcox To: Andrey Ignatov Cc: Christoph Hellwig , Kees Cook , Iurii Zaikin , Luis Chamberlain , Greg Kroah-Hartman , "Rafael J. Wysocki" , Alexei Starovoitov , Daniel Borkmann , linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org Subject: Re: [PATCH 6/6] sysctl: pass kernel pointers to ->proc_handler Message-ID: <20200417195015.GO5820@bombadil.infradead.org> References: <20200417064146.1086644-1-hch@lst.de> <20200417064146.1086644-7-hch@lst.de> <20200417193910.GA7011@rdna-mbp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200417193910.GA7011@rdna-mbp> X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Apr 17, 2020 at 12:39:10PM -0700, Andrey Ignatov wrote: > Though it breaks tools/testing/selftests/bpf/test_sysctl.c. I spent some > time debugging and found a couple of problems -- see below. But there is > something else .. Still I figured it's a good idea to give an early > heads-up. "see below"? Really? You're going to say that and then make people scroll through thousands of lines of quoted material to find your new contributions? Please, learn to trim appropriately. Here's about what you should have sent: > > @@ -1156,52 +1153,41 @@ const struct bpf_verifier_ops cg_dev_verifier_ops = { > > */ > > int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head, > > struct ctl_table *table, int write, > > - void __user *buf, size_t *pcount, > > - loff_t *ppos, void **new_buf, > > - enum bpf_attach_type type) > > + void **buf, size_t *pcount, > > + loff_t *ppos, enum bpf_attach_type type) > > { > > struct bpf_sysctl_kern ctx = { > > .head = head, > > .table = table, > > .write = write, > > .ppos = ppos, > > - .cur_val = NULL, > > + .cur_val = *buf, > > > cur_val is allocated separately below to read current value of sysctl > and not interfere with user-passed buffer. > > > .cur_len = PAGE_SIZE, > > .new_val = NULL, > > .new_len = 0, > > .new_updated = 0, > > }; > > struct cgroup *cgrp; > > + loff_t pos = 0; > > int ret; > > > > - ctx.cur_val = kmalloc_track_caller(ctx.cur_len, GFP_KERNEL); > > - if (ctx.cur_val) { > > - mm_segment_t old_fs; > > - loff_t pos = 0; > > - > > - old_fs = get_fs(); > > - set_fs(KERNEL_DS); > > - if (table->proc_handler(table, 0, (void __user *)ctx.cur_val, > > - &ctx.cur_len, &pos)) { > > - /* Let BPF program decide how to proceed. */ > > - ctx.cur_len = 0; > > - } > > - set_fs(old_fs); > > - } else { > > + if (table->proc_handler(table, 0, ctx.cur_val, &ctx.cur_len, &pos)) { > > This call reads current value of sysclt into cur_val buffer. > > Since you made cur_val point to kernel copy of user-passed buffer, this > call will always override whatever is there in that kernel copy. > > For example, if user is writing to sysclt, then *buf is a pointer to new > value, but this call will override this new value and, corresondingly > new value will be lost. > > I think cur_val should still be allocated separately. > > > > /* Let BPF program decide how to proceed. */ > > ctx.cur_len = 0; > > } > > > > - if (write && buf && *pcount) { > > + if (write && *pcount) { > > /* BPF program should be able to override new value with a > > * buffer bigger than provided by user. > > */ > > ctx.new_val = kmalloc_track_caller(PAGE_SIZE, GFP_KERNEL); > > - ctx.new_len = min_t(size_t, PAGE_SIZE, *pcount); > > - if (!ctx.new_val || > > - copy_from_user(ctx.new_val, buf, ctx.new_len)) > > + if (ctx.new_val) { > > + ctx.new_len = min_t(size_t, PAGE_SIZE, *pcount); > > + memcpy(ctx.new_val, buf, ctx.new_len); > > This should be *buf, not buf. A typo I guess? > > > I applied the whole patchset to bpf-next tree and run selftests. This > patch breaks 4 of them: > > % cd tools/testing/selftests/bpf/ > % ./test_sysctl > ... > Test case: sysctl_get_new_value sysctl:write ok .. [FAIL] > Test case: sysctl_get_new_value sysctl:write ok long .. [FAIL] > Test case: sysctl_get_new_value sysctl:write E2BIG .. [FAIL] > Test case: sysctl_set_new_value sysctl:read EINVAL .. [PASS] > Test case: sysctl_set_new_value sysctl:write ok .. [FAIL] > ... > Summary: 36 PASSED, 4 FAILED > > I applied both changes I suggested above and it reduces number of broken > selftests to one: > > Test case: sysctl_set_new_value sysctl:write ok .. [FAIL] > > I haven't debugged this last one though yet .. > > All these tests are available in > tools/testing/selftests/bpf/test_sysctl.c. > > I think it's a good idea to run these tests locally before sending the > next version of the patch set. >