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=-7.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 74A4BC433E1 for ; Mon, 27 Jul 2020 14:55:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 58046206E7 for ; Mon, 27 Jul 2020 14:55:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729518AbgG0OzX (ORCPT ); Mon, 27 Jul 2020 10:55:23 -0400 Received: from mout.kundenserver.de ([212.227.126.187]:36535 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728593AbgG0OzX (ORCPT ); Mon, 27 Jul 2020 10:55:23 -0400 Received: from mail-qt1-f176.google.com ([209.85.160.176]) by mrelayeu.kundenserver.de (mreue012 [212.227.15.129]) with ESMTPSA (Nemesis) id 1M1rGy-1jxrbk3MBU-002EwH; Mon, 27 Jul 2020 16:55:21 +0200 Received: by mail-qt1-f176.google.com with SMTP id a32so12388268qtb.5; Mon, 27 Jul 2020 07:55:20 -0700 (PDT) X-Gm-Message-State: AOAM533ZWz0enoIgMN0JE8JFROIGl461VIVcMTiHe2Z7S1MsrzAZZ1fy W0aPOd+sqKZ2zAnni0baLsQQ+DfCTt131kJTXJA= X-Google-Smtp-Source: ABdhPJx9V72QTymo8w1nyOPNr9QDIb1f85k08B5c7bfE5ycZlPzb1BLAKjqSaYTOQzF9HCrRgrx1SHcX9er4/B6hPNo= X-Received: by 2002:ac8:4652:: with SMTP id f18mr4923449qto.142.1595861719558; Mon, 27 Jul 2020 07:55:19 -0700 (PDT) MIME-Version: 1.0 References: <20200726220557.102300-1-yepeilin.cs@gmail.com> <20200726222703.102701-1-yepeilin.cs@gmail.com> <20200727131608.GD1913@kadam> <20200727144335.GB2571@kadam> In-Reply-To: <20200727144335.GB2571@kadam> From: Arnd Bergmann Date: Mon, 27 Jul 2020 16:55:03 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user() To: Dan Carpenter Cc: Peilin Ye , Mauro Carvalho Chehab , Greg Kroah-Hartman , syzkaller-bugs , Hans Verkuil , Sakari Ailus , Laurent Pinchart , Vandana BN , Ezequiel Garcia , =?UTF-8?Q?Niklas_S=C3=B6derlund?= , linux-kernel-mentees@lists.linuxfoundation.org, Linux Media Mailing List , "linux-kernel@vger.kernel.org" , Linus Walleij Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:tg/AoHCEOzFAvg7gpivm7llDARO5ETgYItX/UyNLv11Hj+kUEss gaoEZqF9Swg/frhxewZMg9np3RCE8raNgC8ifNllb/2kfx9JXS/lpMeXQj+12lADYBftr4C 5ZnjmUFYNLZ2i/SKgmvJ3NpOxAVewrdOxuX169B0fKxB5Joo/4933kfNlVhejQc+DYsIkdT QfzF1CVkSuqzlvs0yjJ4A== X-UI-Out-Filterresults: notjunk:1;V03:K0:/gJ4HQPTSlA=:wK7KjLlZvMxrMBqGlgrOEl C6EwE3AgVo1zSP7IpVa33AMGlCVKQg5Zc5b9knmZCKjhTTey8rr324tkm4gRqtg6zOn7tHg8v uZfE4F8RVgfydtO7QcBkqd3ThzLh34apHEeAE+czrGQMiPrD1RYdni9lxILFSEvQ+TluXsaZI +6fZFucgrJlAYi6uTL1rm24BQZ0S4YIl2qh42fBL2yo2yO7gF785b/QidG/euLr8nEJlnrC+e ybyI+PGXUaHkqvL+vnzNGHzFu4bkFpMK83qR5NnLfyQw+qsTQC4s3EJ54wZ2fyPbTvHlQi5FV 7eIHAwkCI9z9gFD8RBGP3679fF3w+KhW7fUQwMDQrIeflsgLLwjaZZCdfkfF5WPbksoX64U9u f6VTTxWTt5CLVNzoOmv2Vvvd5KQKzSSYaX9+p1QJeDZ0mWP35c6Ll/jhqDgEK1VZ9+W0nhxJB y7c8hcpMbS/0rthFWxJkrcwU+rBHgRHf92zf1lMd71o0ZEQz5+1h6WCEnZ5isX4D6ou+pACeO yiOPOn1K9kLNB6HupDV9qO5CX3SGV7yTJbaouNVcQ5rxEUFYAExaNxEX7P3adupDVxPX6oE2A 3evrpnfcgNPf2TvbGOa+yCq24tELJ3054emLX/JE35o0kvIg36miAcSUOGOVawj/yr+QRx9qn TjKz2c6A7iKOBVbumWnUDkznQnH3s105E36io3PEmrxHoOPzcuy1j7aHWKPMvHRMHNgr02ds9 8hOf+N9I0cp1rED498wkQfcRm19Cr+539pKmooSvRWipVtCZDAM4Q13WMaJ0kusE58GHM9ncs Hyyw/oX+bYYy01LUdJ7lBed2eyMFtMDPpxsImOQGc2ZrwRU3WlOi+oCKT4HrMTkOFsZFIw1cw H/ERaHBb2Lut0TYrgrsIQo83fZVABCuU4aaClyF5VXn/rjJSNluW8GGdg0PZLnisI2zYThrTa lwLC2cn7oHiSCyeUHOnkIGFuSZSmhuoQRQkYEz+8n12UyceBN5ADh Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 27, 2020 at 4:43 PM Dan Carpenter wrote: > > On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote: > > On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter wrote: > > > > > > On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote: > > > > On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye wrote: > > > > > > > > > > video_put_user() is copying uninitialized stack memory to userspace due > > > > > to the compiler not initializing holes in the structures declared on the > > > > > stack. Fix it by initializing `ev32` and `vb32` using memset(). > > > > > > > > > > Reported-and-tested-by: syzbot+79d751604cb6f29fbf59@syzkaller.appspotmail.com > > > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59 > > > > > Reviewed-by: Laurent Pinchart > > > > > Signed-off-by: Peilin Ye > > > > > > > > Thanks a lot for addressing this! I now see that I actually created a similar > > > > bugfix for it back in January, but for some reason that got stuck in my > > > > backlog and I never wrote a proper description for it or sent it out to the > > > > list, sorry about that. I would hope we could find a way to have either > > > > the compiler or sparse warn if we copy uninitialized data to user space, > > > > but we now don't even check for that within the kernel any more. > > > > > > Here are my latest warnings on linux-next from Friday. > > > > Ah, I forgot you had that kind of list already, thanks for checking! > > > > > block/scsi_ioctl.c:707 scsi_put_cdrom_generic_arg() warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction') > > > > I see no padding in this one, should be fine AFAICT. Any idea why you > > get a warning > > for this instance? > > > > The warning message only prints the first struct hole or uninitialized > struct memeber. ->data_direction in this case. > > block/scsi_ioctl.c > 646 #ifdef CONFIG_COMPAT > 647 struct compat_cdrom_generic_command { > 648 unsigned char cmd[CDROM_PACKET_SIZE]; > 649 compat_caddr_t buffer; > 650 compat_uint_t buflen; > 651 compat_int_t stat; > 652 compat_caddr_t sense; > 653 unsigned char data_direction; > > There is going to be 3 bytes of padding between this char and the next > int. > > 654 compat_int_t quiet; > 655 compat_int_t timeout; > 656 compat_caddr_t reserved[1]; > 657 }; > 658 #endif > > The bug seems real, but it depends on the compiler of course. Right, I misread the single 'char' in there. > > > drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay') > > > > This one hs padding in it and looks broken. > > No. This a bug in smatch. It's memcpy() over the hole, but the check > isn't capturing that. The code is slightly weird... I could try > silence the warning but it would only silence this one false positive so > I haven't investigated it. Ah right, and the structure it copies in turn comes from user space originally. > > > drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information > > > > Seems fine due to __packed annotation. > > > > It's not related __packed. Smatch is saying that cinfo.base isn't > necessarily initialized. > > drivers/scsi/megaraid/megaraid_mm.c > 816 > 817 case MEGAIOC_QADAPINFO: > 818 > 819 hinfo = (mraid_hba_info_t *)(unsigned long) > 820 kioc->buf_vaddr; > 821 > 822 hinfo_to_cinfo(hinfo, &cinfo); > > hinfo_to_cinfo() is a no-op if hinfo is NULL. Smatch can't tell if > "hinfo" is non-NULL. Ok, that sounds fair, I couldn't easily tell either ;-) Arnd 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=-7.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 DE30AC433E3 for ; Mon, 27 Jul 2020 14:55:29 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (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 AFD23206E7 for ; Mon, 27 Jul 2020 14:55:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AFD23206E7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arndb.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linux-kernel-mentees-bounces@lists.linuxfoundation.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 7413E20423; Mon, 27 Jul 2020 14:55:29 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dGJtfn04hi0Z; Mon, 27 Jul 2020 14:55:26 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 35AC1203B2; Mon, 27 Jul 2020 14:55:26 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 2038EC004F; Mon, 27 Jul 2020 14:55:26 +0000 (UTC) Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 75E88C004D for ; Mon, 27 Jul 2020 14:55:25 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 650A98142C for ; Mon, 27 Jul 2020 14:55:25 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7vd7WPyd4ezy for ; Mon, 27 Jul 2020 14:55:24 +0000 (UTC) X-Greylist: from auto-whitelisted by SQLgrey-1.7.6 Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.135]) by whitealder.osuosl.org (Postfix) with ESMTPS id D78FB875BF for ; Mon, 27 Jul 2020 14:55:23 +0000 (UTC) Received: from mail-qt1-f177.google.com ([209.85.160.177]) by mrelayeu.kundenserver.de (mreue011 [212.227.15.129]) with ESMTPSA (Nemesis) id 1M9nhF-1jus053UZx-005sV0 for ; Mon, 27 Jul 2020 16:55:21 +0200 Received: by mail-qt1-f177.google.com with SMTP id s23so12355223qtq.12 for ; Mon, 27 Jul 2020 07:55:20 -0700 (PDT) X-Gm-Message-State: AOAM531Ha8LlRVAlUBUkFvz4mc+PEkKHkWBKeLJhVnYEuKkwSI7yT0rZ 3sa/8hz4ws6LM3VjKdXHk2pOJ+aLSGxK1O/Kyng= X-Google-Smtp-Source: ABdhPJx9V72QTymo8w1nyOPNr9QDIb1f85k08B5c7bfE5ycZlPzb1BLAKjqSaYTOQzF9HCrRgrx1SHcX9er4/B6hPNo= X-Received: by 2002:ac8:4652:: with SMTP id f18mr4923449qto.142.1595861719558; Mon, 27 Jul 2020 07:55:19 -0700 (PDT) MIME-Version: 1.0 References: <20200726220557.102300-1-yepeilin.cs@gmail.com> <20200726222703.102701-1-yepeilin.cs@gmail.com> <20200727131608.GD1913@kadam> <20200727144335.GB2571@kadam> In-Reply-To: <20200727144335.GB2571@kadam> From: Arnd Bergmann Date: Mon, 27 Jul 2020 16:55:03 +0200 X-Gmail-Original-Message-ID: Message-ID: To: Dan Carpenter X-Provags-ID: V03:K1:zUuocsOIlqDwpJNwWlLUfs7YyP6gNPvrWIUytR6d3anBgqcMfpb hIMMU9yDtrDdmwxkCSUzWAz/dpQRdEbAwrYnwubNDefWW/Ov9c4qJrluibNzDjiiDMrgmJF yarku98jf57OBv04HKzpzKihsq6R6Wp9ZCn8YMgLr5CNEtVwnTKG+UAfwqrTnpLJxAjmVZ1 Dy7TqmWAnsXP88uI5RT2Q== X-UI-Out-Filterresults: notjunk:1;V03:K0:NrmRKEQggWk=:iQxMKtb4VMsJ5Cqz11RFl+ z926i7kpS5KAPgT+lY8Zvhzvq+E9vZ7kTJfWJ6tkVbAGVYIRKZhUf+71uiUuGxdAVXqUD/Gvl pyqQwiWEq4SeTlOwzsEqh1f9URWN7A3R51hMtwnbsvZ1XcVYPmKc8qMurcrczTocu2tlQY+mc Y+yPZQSQActU+OEUEF/qIo+5OmQsKNLLJFsIZzIJ7CePU8SfzZ8wXZIn0Rra0dTpYWSl1LorM nyfnbqnu3j0s3yAP7YCK2l5NYIggsJgdx8FB71CFZD7b1xJWm7uqoJWLK4DLqugRoCt/RFoFm g3o3bjJcsSrhaJ5k37L/dAX6OHgajN3CqyteO5GWfJcziwILNzv+B8J1Dzj+39SWOq6nAe9Qy 0SBviM59Z8SprSh6JQonrFFxByyHqMyZapzJ26nNBMx2wQEeHAqj4vOm/vgTY5a239qQKtfoZ vGu5Wh8ye5aZ8uM6QorZ4RZaJzHfkcUEi8hxh7gEi2g9cOOVFL+TEh2VAGLdbXFPkFjpvnFh5 3ekCG305YCRrXp+nXnIooBoV87dKvLOCk7LlpzkNxycpXIFfkNHfrRW2wIU2VIFHIxobpim+J uZHOkx9nAo1KygVvpOZsi5oyPRuW+i/hWHw2nkt02D0kA1w1TxA/I38bF39Qqn+asHfOTXxdm Rk/1UTehzETwHliZsSwZOhxa3Ewmj9GXCwSLuO7p0G1F1yyj01pA8Wnh25Ebeo2sU1/yyNHBS P7o/7DtwNRU9bHAnb0TYkzimmr3mNBmvZO2P2nju59muikhR+An6LBoP7/AEUZeG4e3NAFMV/ 7+ufvdaoVeEbTR+kNDOJYZ/ZuuPc25NEdvKb14dRdj3EWekmSh8LV793frr0ilpz0SRyFbo/J wG+uv78Kec7Un9WMEceh6nxiJzEvpQdIHGngxLNjOtIDy6YzaD4sBO5tBpWPTxf0/Ze/wTXvT EYUyniQGOaxJTBvcZ6zZrecl6R9Qz1gnhYzzC/IADUqe8/8KYlgWT Cc: =?UTF-8?Q?Niklas_S=C3=B6derlund?= , Linus Walleij , syzkaller-bugs , Linux Media Mailing List , "linux-kernel@vger.kernel.org" , Laurent Pinchart , Sakari Ailus , Vandana BN , Hans Verkuil , Mauro Carvalho Chehab , Ezequiel Garcia , Peilin Ye , linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [Linux-kernel-mentees] [PATCH v3] media/v4l2-core: Fix kernel-infoleak in video_put_user() X-BeenThere: linux-kernel-mentees@lists.linuxfoundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-kernel-mentees-bounces@lists.linuxfoundation.org Sender: "Linux-kernel-mentees" On Mon, Jul 27, 2020 at 4:43 PM Dan Carpenter wrote: > > On Mon, Jul 27, 2020 at 04:05:38PM +0200, Arnd Bergmann wrote: > > On Mon, Jul 27, 2020 at 3:16 PM Dan Carpenter wrote: > > > > > > On Mon, Jul 27, 2020 at 09:25:16AM +0200, Arnd Bergmann wrote: > > > > On Mon, Jul 27, 2020 at 12:28 AM Peilin Ye wrote: > > > > > > > > > > video_put_user() is copying uninitialized stack memory to userspace due > > > > > to the compiler not initializing holes in the structures declared on the > > > > > stack. Fix it by initializing `ev32` and `vb32` using memset(). > > > > > > > > > > Reported-and-tested-by: syzbot+79d751604cb6f29fbf59@syzkaller.appspotmail.com > > > > > Link: https://syzkaller.appspot.com/bug?extid=79d751604cb6f29fbf59 > > > > > Reviewed-by: Laurent Pinchart > > > > > Signed-off-by: Peilin Ye > > > > > > > > Thanks a lot for addressing this! I now see that I actually created a similar > > > > bugfix for it back in January, but for some reason that got stuck in my > > > > backlog and I never wrote a proper description for it or sent it out to the > > > > list, sorry about that. I would hope we could find a way to have either > > > > the compiler or sparse warn if we copy uninitialized data to user space, > > > > but we now don't even check for that within the kernel any more. > > > > > > Here are my latest warnings on linux-next from Friday. > > > > Ah, I forgot you had that kind of list already, thanks for checking! > > > > > block/scsi_ioctl.c:707 scsi_put_cdrom_generic_arg() warn: check that 'cgc32' doesn't leak information (struct has a hole after 'data_direction') > > > > I see no padding in this one, should be fine AFAICT. Any idea why you > > get a warning > > for this instance? > > > > The warning message only prints the first struct hole or uninitialized > struct memeber. ->data_direction in this case. > > block/scsi_ioctl.c > 646 #ifdef CONFIG_COMPAT > 647 struct compat_cdrom_generic_command { > 648 unsigned char cmd[CDROM_PACKET_SIZE]; > 649 compat_caddr_t buffer; > 650 compat_uint_t buflen; > 651 compat_int_t stat; > 652 compat_caddr_t sense; > 653 unsigned char data_direction; > > There is going to be 3 bytes of padding between this char and the next > int. > > 654 compat_int_t quiet; > 655 compat_int_t timeout; > 656 compat_caddr_t reserved[1]; > 657 }; > 658 #endif > > The bug seems real, but it depends on the compiler of course. Right, I misread the single 'char' in there. > > > drivers/input/misc/uinput.c:743 uinput_ff_upload_to_user() warn: check that 'ff_up_compat' doesn't leak information (struct has a hole after 'replay') > > > > This one hs padding in it and looks broken. > > No. This a bug in smatch. It's memcpy() over the hole, but the check > isn't capturing that. The code is slightly weird... I could try > silence the warning but it would only silence this one false positive so > I haven't investigated it. Ah right, and the structure it copies in turn comes from user space originally. > > > drivers/scsi/megaraid/megaraid_mm.c:824 kioc_to_mimd() warn: check that 'cinfo.base' doesn't leak information > > > > Seems fine due to __packed annotation. > > > > It's not related __packed. Smatch is saying that cinfo.base isn't > necessarily initialized. > > drivers/scsi/megaraid/megaraid_mm.c > 816 > 817 case MEGAIOC_QADAPINFO: > 818 > 819 hinfo = (mraid_hba_info_t *)(unsigned long) > 820 kioc->buf_vaddr; > 821 > 822 hinfo_to_cinfo(hinfo, &cinfo); > > hinfo_to_cinfo() is a no-op if hinfo is NULL. Smatch can't tell if > "hinfo" is non-NULL. Ok, that sounds fair, I couldn't easily tell either ;-) Arnd _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees