From patchwork Wed Jul 11 00:40:29 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 10518425 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 606AE6032C for ; Wed, 11 Jul 2018 00:40:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 50A1C28D6D for ; Wed, 11 Jul 2018 00:40:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 44F4328DC3; Wed, 11 Jul 2018 00:40:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B0AFE28D6D for ; Wed, 11 Jul 2018 00:40:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732264AbeGKAmR (ORCPT ); Tue, 10 Jul 2018 20:42:17 -0400 Received: from mail-io0-f193.google.com ([209.85.223.193]:36978 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732262AbeGKAmR (ORCPT ); Tue, 10 Jul 2018 20:42:17 -0400 Received: by mail-io0-f193.google.com with SMTP id z19-v6so22080654ioh.4; Tue, 10 Jul 2018 17:40:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=NEgtF0qoaOMYkYzqkSAVkMqdFB+vsXarEKaOYiZgf7I=; b=JUcMcsXU7f2zEE7eZpASIywpgizHJ9Je2Dq5dPl4QTpaak5OzXWIjPzxBHtg9LK0+3 mCc33Bk3qF380zl49rML6zrTUoiaoSySPEnF4YTdHvmBwj9fKTraNtJhyJnQu+7DdMX1 kXkaflYteqE17IvcDssvt+ZhA2SPRnBxrMwrI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=NEgtF0qoaOMYkYzqkSAVkMqdFB+vsXarEKaOYiZgf7I=; b=VkJmKZDd0j+p9MvanSsHb+/7d8ur6t89V4/WSXoFKDzgo6A0c/xdqOhGPiBUBQeyjz qRYDdIiNUJMlhJYfcDkfScOorKWHHNW82ntATYbo4q7Nf0LxOniF/lOm5biY2xtVYN3R Bvl1QEAJ+/6wVH+d2IhjMSAiJ27SgK++yliQST0b+guQggsHk7R1VR9eKqWCZH9wiyWi YBUY7OhKg+4ZTesFkYLSegp/PUmloxri1abV0nSl1Md9EVcKvHcCD2vnjV9x53BnWvkr eeBTcAq8J9J3QnM2oDOIHB8XwHMHgA4oRXBSICoDRq6BjeKJZDI7BzyvWE/LL7FP3B4Q FkbA== X-Gm-Message-State: APt69E3Eemg6YdzZ0bVR8cGZqAV9fOOiKRmcsiGpuBcx4HpcpK3R7SOk 698HR/r1bppWTt8MTyFROmJI+Qc2N5qnzEQDqpg= X-Google-Smtp-Source: AAOMgpdwILN8V5jGdRddO1ERgg1Frr1pfc/mompwYOuKwKSMjQAvWll2d0LwGhv4npFVkVmBJWA+VRr3agxr1UH2Z0U= X-Received: by 2002:a6b:7a05:: with SMTP id h5-v6mr7598063iom.238.1531269640623; Tue, 10 Jul 2018 17:40:40 -0700 (PDT) MIME-Version: 1.0 References: <1530913134.3135.2.camel@HansenPartnership.com> <1530940958.3135.4.camel@HansenPartnership.com> <70d566f0-2dda-60c2-7c7a-e09e371f8607@cybernetics.com> In-Reply-To: From: Linus Torvalds Date: Tue, 10 Jul 2018 17:40:29 -0700 Message-ID: Subject: Re: Re: [GIT PULL] SCSI fixes for 4.18-rc3 To: tonyb@cybernetics.com Cc: James Bottomley , Jann Horn , Andrew Morton , Linux SCSI List , Linux Kernel Mailing List Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 10, 2018 at 3:24 PM Linus Torvalds wrote: > > So /dev/sg really has serious issues. Not just the read/write part, > but even the SG_IO part is broken right now (sg_new_write() actually > does do blk_verify_command(), but only for read-only opens for some > reason). Looking more at this, it's even more broken than that. Look at SCSI_IOCTL_SEND_COMMAND. It will do a sg_scsi_ioctl(), but before that it does the same crazy sg_allow_access() case - and only for read-only opens. But that's *doubly* wrong, because (a) it's racy, and does the command check on a local copy that might not be the final one. (b) it's pointless, because sg_scsi_ioctl() actually does the proper command check on the final command as it was copied from user space. And yes, sg_scsi_ioctl() itself has a slight race in that first it reads the first byte of the command ("opcode") in order to look up the size of the command, and then it reads the whole command - including the first byte - again. So it has the same "read twice, use possibly inconsistent data" issue, but the actual command that is checked is the final one that matches the command that is actually submitted. So all you can do is confuse "cmdlen" and then get timeouts and retry attempts based on the "fist command" value, but the actually sent command is fine. I'm not even going to bother with the sg_scsi_ioctl() race, because it's harmless, but the drivers/scsi/sg.c code is just pointless and confusing and should be removed. Something like the attached? Please, can we try to at least just de-crapify some of this code? There's that other "sg_allow_access()" that I also think is wrong and should just use blk_verify_command() directly and even when opened for read, but that whole "read or TYPE_SCANNER" is presumably some special crazy hack for some odd SCSI device. Linus From f075dce66c6039bb97b762b592a6db6d79e4350a Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Tue, 10 Jul 2018 17:02:17 -0700 Subject: [PATCH] scsi sg: remove incorrect scsi command checking logic The SCSI_IOCTL_SEND_COMMAND ioctl has interesting scsi command "security" checking. If the file was opened read-only (but only in that case), it will fetch the first byte of the command from user space, and do "sg_allow_access()" on it. That, in turn, will check that "blk_verify_command()" is ok with that command byte. If that passes, it will then do call "sg_scsi_ioctl()" to execute the command. This is entirely nonsensical for several reasons. It's nonsensical simply because it's racy: after it copies the command byte from user mode to check it, user mode could just change the byte before it is actually submitted later by "sg_scsi_ioctl()". But it is nonsensical also because "sg_scsi_ioctl()" itself already does blk_verify_command() on the command properly after it has been copied from user space. So it is an incorrect implementation of a pointless check. Remove it. Signed-off-by: Linus Torvalds --- drivers/scsi/sg.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index cd2fdac000c9..4f3450793273 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -1103,15 +1103,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg) case SCSI_IOCTL_SEND_COMMAND: if (atomic_read(&sdp->detaching)) return -ENODEV; - if (read_only) { - unsigned char opcode = WRITE_6; - Scsi_Ioctl_Command __user *siocp = p; - - if (copy_from_user(&opcode, siocp->data, 1)) - return -EFAULT; - if (sg_allow_access(filp, &opcode)) - return -EPERM; - } return sg_scsi_ioctl(sdp->device->request_queue, NULL, filp->f_mode, p); case SG_SET_DEBUG: result = get_user(val, ip); -- 2.18.0.132.g95eda3d86