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 DAA5FCA9EAE for ; Tue, 29 Oct 2019 19:02:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AEF3C20679 for ; Tue, 29 Oct 2019 19:02:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729079AbfJ2TC1 (ORCPT ); Tue, 29 Oct 2019 15:02:27 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:48059 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726183AbfJ2TC1 (ORCPT ); Tue, 29 Oct 2019 15:02:27 -0400 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 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 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 Content-Type: text/plain 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) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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)