From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Date: Tue, 29 Oct 2019 19:02:11 +0000 Subject: Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() Message-Id: <87zhhjjryk.fsf@x220.int.ebiederm.org> List-Id: References: <20191029182320.GA17569@mwanda> In-Reply-To: <20191029182320.GA17569@mwanda> (Dan Carpenter's message of "Tue, 29 Oct 2019 21:23:20 +0300") MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Bartlomiej Zolnierkiewicz , Andrea Righi , Daniel Vetter , Sam Ravnborg , Maarten Lankhorst , Peter Rosin , Gerd Hoffmann , dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, security@kernel.org, Kees Cook , Julia Lawall Dan Carpenter writes: > The "fix" struct has a 2 byte hole after ->ywrapstep and the > "fix = info->fix;" assignment doesn't necessarily clear it. It depends > on the compiler. > > Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held") > Signed-off-by: Dan Carpenter > --- > I have 13 more similar places to patch... I'm not totally sure I > understand all the issues involved. What I have done in a similar situation with struct siginfo, is that where the structure first appears I have initialized it with memset, and then field by field. Then when the structure is copied I copy the structure with memcpy. That ensures all of the bytes in the original structure are initialized and that all of the bytes are copied. The goal is to avoid memory that has values of the previous users of that memory region from leaking to userspace. Which depending on who the previous user of that memory region is could tell userspace information about what the kernel is doing that it should not be allowed to find out. I tried to trace through where "info" and thus presumably "info->fix" is coming from and only made it as far as register_framebuffer. Given that I suspect a local memset, and then a field by field copy right before copy_to_user might be a sound solution. But ick. That is a lot of fields to copy. Eric > drivers/video/fbdev/core/fbmem.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 6f6fc785b545..b4ce6a28aed9 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1109,6 +1109,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > ret = -EFAULT; > break; > case FBIOGET_FSCREENINFO: > + memset(&fix, 0, sizeof(fix)); > lock_fb_info(info); > fix = info->fix; > if (info->flags & FBINFO_HIDE_SMEM_START) From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() Date: Tue, 29 Oct 2019 14:02:11 -0500 Message-ID: <87zhhjjryk.fsf@x220.int.ebiederm.org> References: <20191029182320.GA17569@mwanda> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: <20191029182320.GA17569@mwanda> (Dan Carpenter's message of "Tue, 29 Oct 2019 21:23:20 +0300") Sender: linux-kernel-owner@vger.kernel.org To: Dan Carpenter Cc: Bartlomiej Zolnierkiewicz , Andrea Righi , Daniel Vetter , Sam Ravnborg , Maarten Lankhorst , Peter Rosin , Gerd Hoffmann , dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, security@kernel.org, Kees Cook , Julia Lawall List-Id: dri-devel@lists.freedesktop.org Dan Carpenter writes: > The "fix" struct has a 2 byte hole after ->ywrapstep and the > "fix = info->fix;" assignment doesn't necessarily clear it. It depends > on the compiler. > > Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held") > Signed-off-by: Dan Carpenter > --- > I have 13 more similar places to patch... I'm not totally sure I > understand all the issues involved. What I have done in a similar situation with struct siginfo, is that where the structure first appears I have initialized it with memset, and then field by field. Then when the structure is copied I copy the structure with memcpy. That ensures all of the bytes in the original structure are initialized and that all of the bytes are copied. The goal is to avoid memory that has values of the previous users of that memory region from leaking to userspace. Which depending on who the previous user of that memory region is could tell userspace information about what the kernel is doing that it should not be allowed to find out. I tried to trace through where "info" and thus presumably "info->fix" is coming from and only made it as far as register_framebuffer. Given that I suspect a local memset, and then a field by field copy right before copy_to_user might be a sound solution. But ick. That is a lot of fields to copy. Eric > drivers/video/fbdev/core/fbmem.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 6f6fc785b545..b4ce6a28aed9 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1109,6 +1109,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > ret = -EFAULT; > break; > case FBIOGET_FSCREENINFO: > + memset(&fix, 0, sizeof(fix)); > lock_fb_info(info); > fix = info->fix; > if (info->flags & FBINFO_HIDE_SMEM_START) 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=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 0B231CA9EAF for ; Wed, 30 Oct 2019 08:15:27 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DC8732067D for ; Wed, 30 Oct 2019 08:15:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DC8732067D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B5686ECC7; Wed, 30 Oct 2019 08:14:06 +0000 (UTC) X-Greylist: delayed 1763 seconds by postgrey-1.36 at gabe; Tue, 29 Oct 2019 19:31:49 UTC Received: from out02.mta.xmission.com (out02.mta.xmission.com [166.70.13.232]) by gabe.freedesktop.org (Postfix) with ESMTPS id 404736E5D9 for ; Tue, 29 Oct 2019 19:31:49 +0000 (UTC) Received: from in02.mta.xmission.com ([166.70.13.52]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1iPWl0-0004EV-T4; Tue, 29 Oct 2019 13:02:18 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1iPWl0-0006xT-3i; Tue, 29 Oct 2019 13:02:18 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Dan Carpenter References: <20191029182320.GA17569@mwanda> Date: Tue, 29 Oct 2019 14:02:11 -0500 In-Reply-To: <20191029182320.GA17569@mwanda> (Dan Carpenter's message of "Tue, 29 Oct 2019 21:23:20 +0300") Message-ID: <87zhhjjryk.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1iPWl0-0006xT-3i; ; ; mid=<87zhhjjryk.fsf@x220.int.ebiederm.org>; ; ; hst=in02.mta.xmission.com; ; ; ip=68.227.160.95; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX18PHyqhwTatudH41XJqOC8sC0I5jQ9SR2U= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-Mailman-Approved-At: Wed, 30 Oct 2019 08:14:00 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, security@kernel.org, Kees Cook , Bartlomiej Zolnierkiewicz , kernel-janitors@vger.kernel.org, Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Julia Lawall , Gerd Hoffmann , Sam Ravnborg , Peter Rosin , Andrea Righi Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Message-ID: <20191029190211.G0ruoaWCSHuUnceJKmYI4XVqEVXmbnAyPrI8Ol1NHLs@z> RGFuIENhcnBlbnRlciA8ZGFuLmNhcnBlbnRlckBvcmFjbGUuY29tPiB3cml0ZXM6Cgo+IFRoZSAi Zml4IiBzdHJ1Y3QgaGFzIGEgMiBieXRlIGhvbGUgYWZ0ZXIgLT55d3JhcHN0ZXAgYW5kIHRoZQo+ ICJmaXggPSBpbmZvLT5maXg7IiBhc3NpZ25tZW50IGRvZXNuJ3QgbmVjZXNzYXJpbHkgY2xlYXIg aXQuICBJdCBkZXBlbmRzCj4gb24gdGhlIGNvbXBpbGVyLgo+Cj4gRml4ZXM6IDFmNWUzMWQ3ZTU1 YSAoImZibWVtOiBkb24ndCBjYWxsIGNvcHlfZnJvbS90b191c2VyKCkgd2l0aCBtdXRleCBoZWxk IikKPiBTaWduZWQtb2ZmLWJ5OiBEYW4gQ2FycGVudGVyIDxkYW4uY2FycGVudGVyQG9yYWNsZS5j b20+Cj4gLS0tCj4gSSBoYXZlIDEzIG1vcmUgc2ltaWxhciBwbGFjZXMgdG8gcGF0Y2guLi4gIEkn bSBub3QgdG90YWxseSBzdXJlIEkKPiB1bmRlcnN0YW5kIGFsbCB0aGUgaXNzdWVzIGludm9sdmVk LgoKV2hhdCBJIGhhdmUgZG9uZSBpbiBhIHNpbWlsYXIgc2l0dWF0aW9uIHdpdGggc3RydWN0IHNp Z2luZm8sIGlzIHRoYXQKd2hlcmUgdGhlIHN0cnVjdHVyZSBmaXJzdCBhcHBlYXJzIEkgaGF2ZSBp bml0aWFsaXplZCBpdCB3aXRoIG1lbXNldCwKYW5kIHRoZW4gZmllbGQgYnkgZmllbGQuCgpUaGVu IHdoZW4gdGhlIHN0cnVjdHVyZSBpcyBjb3BpZWQgSSBjb3B5IHRoZSBzdHJ1Y3R1cmUgd2l0aCBt ZW1jcHkuCgpUaGF0IGVuc3VyZXMgYWxsIG9mIHRoZSBieXRlcyBpbiB0aGUgb3JpZ2luYWwgc3Ry dWN0dXJlIGFyZSBpbml0aWFsaXplZAphbmQgdGhhdCBhbGwgb2YgdGhlIGJ5dGVzIGFyZSBjb3Bp ZWQuCgpUaGUgZ29hbCBpcyB0byBhdm9pZCBtZW1vcnkgdGhhdCBoYXMgdmFsdWVzIG9mIHRoZSBw cmV2aW91cyB1c2VycyBvZgp0aGF0IG1lbW9yeSByZWdpb24gZnJvbSBsZWFraW5nIHRvIHVzZXJz cGFjZS4gIFdoaWNoIGRlcGVuZGluZyBvbiB3aG8KdGhlIHByZXZpb3VzIHVzZXIgb2YgdGhhdCBt ZW1vcnkgcmVnaW9uIGlzIGNvdWxkIHRlbGwgdXNlcnNwYWNlCmluZm9ybWF0aW9uIGFib3V0IHdo YXQgdGhlIGtlcm5lbCBpcyBkb2luZyB0aGF0IGl0IHNob3VsZCBub3QgYmUgYWxsb3dlZAp0byBm aW5kIG91dC4KCkkgdHJpZWQgdG8gdHJhY2UgdGhyb3VnaCB3aGVyZSAiaW5mbyIgYW5kIHRodXMg cHJlc3VtYWJseSAiaW5mby0+Zml4IiBpcwpjb21pbmcgZnJvbSBhbmQgb25seSBtYWRlIGl0IGFz IGZhciBhcyAgcmVnaXN0ZXJfZnJhbWVidWZmZXIuICBHaXZlbgp0aGF0IEkgc3VzcGVjdCBhIGxv Y2FsIG1lbXNldCwgYW5kIHRoZW4gYSBmaWVsZCBieSBmaWVsZCBjb3B5IHJpZ2h0CmJlZm9yZSBj b3B5X3RvX3VzZXIgbWlnaHQgYmUgYSBzb3VuZCBzb2x1dGlvbi4gIEJ1dCBpY2suICBUaGF0IGlz IGEgbG90Cm9mIGZpZWxkcyB0byBjb3B5LgoKCkVyaWMKCgoKPiAgZHJpdmVycy92aWRlby9mYmRl di9jb3JlL2ZibWVtLmMgfCAxICsKPiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspCj4K PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9mYmRldi9jb3JlL2ZibWVtLmMgYi9kcml2ZXJz L3ZpZGVvL2ZiZGV2L2NvcmUvZmJtZW0uYwo+IGluZGV4IDZmNmZjNzg1YjU0NS4uYjRjZTZhMjhh ZWQ5IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvdmlkZW8vZmJkZXYvY29yZS9mYm1lbS5jCj4gKysr IGIvZHJpdmVycy92aWRlby9mYmRldi9jb3JlL2ZibWVtLmMKPiBAQCAtMTEwOSw2ICsxMTA5LDcg QEAgc3RhdGljIGxvbmcgZG9fZmJfaW9jdGwoc3RydWN0IGZiX2luZm8gKmluZm8sIHVuc2lnbmVk IGludCBjbWQsCj4gIAkJCXJldCA9IC1FRkFVTFQ7Cj4gIAkJYnJlYWs7Cj4gIAljYXNlIEZCSU9H RVRfRlNDUkVFTklORk86Cj4gKwkJbWVtc2V0KCZmaXgsIDAsIHNpemVvZihmaXgpKTsKPiAgCQls b2NrX2ZiX2luZm8oaW5mbyk7Cj4gIAkJZml4ID0gaW5mby0+Zml4Owo+ICAJCWlmIChpbmZvLT5m bGFncyAmIEZCSU5GT19ISURFX1NNRU1fU1RBUlQpCl9fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxp c3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFu L2xpc3RpbmZvL2RyaS1kZXZlbA==