From patchwork Thu Jun 21 15:18:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jann Horn X-Patchwork-Id: 10480067 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 9657A60365 for ; Thu, 21 Jun 2018 15:33:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8669E204FB for ; Thu, 21 Jun 2018 15:33:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7A80926E76; Thu, 21 Jun 2018 15:33:39 +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=-12.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, USER_IN_DEF_DKIM_WL autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id 9498B204FB for ; Thu, 21 Jun 2018 15:33:38 +0000 (UTC) Received: (qmail 31879 invoked by uid 550); 21 Jun 2018 15:33:31 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Delivered-To: moderator for kernel-hardening@lists.openwall.com Received: (qmail 21701 invoked from network); 21 Jun 2018 15:18:32 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:date:message-id:subject:from:to:cc; bh=fd3RD7cjuYUZNJRvBnWqOKvcW146LuGjoL139TL5knc=; b=Ac+b5mBziZ0hF5aseIufrVN6o000IIp6EKbsw67gK/cqA6tp8brPRX9gj3BwYlIsTJ NhHXyqplDUkk5nJkndpaF4amylxcaQfQVC7YJgCTlwSto9PGL2oZlooHMeVhcLg0nGam Z1zBTrfUyf1yYgXnckQhWOxIoYYZcSdEoS6jC3/Wf+tJuc4ukfMPZvFqnUofa2glK8NA 0GW8c+t/0+mXYErHvHj3Kuhqqd4iFQxCSKR6TzPPwwjIg8iBgJPJkQy2g3VKRMYJhU/8 UW2xywD7tYrYPIx/41dgmQZWjCQ0UkNAALZezqb+w6lPxlCJTEDtYHC5A84rDlTRJyUw f94Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:date:message-id:subject:from:to:cc; bh=fd3RD7cjuYUZNJRvBnWqOKvcW146LuGjoL139TL5knc=; b=FPbhCu1IZK7QQm8g2LWX24/l0kGSfDWAose9Pq0kfTdpkBM5DHb0sDd9xGYwQGT9VV nyZnvq9YvylruWiRLVQhlifJzX2Yng7u8MlDvmGePOg1TOXmPQIrtAb4zBRL9sMTeASb hit8wyRKTnSaleVvkLBfMbz9ou07s+ofbIPBZ/eZdx9e1f4AbwHxqqHvBPtbJtOxX1wO tKGaIfZ9RztePOywG7nfAKWuLpMdI/pEuKv2nsTL6fAgTMVCidqqwRvYDeJvoN6JSUP4 V1Z8ABP0T5ZbtyByrCtS/mI/0qalNlYD5sEEZ1q9G2U7lUp3DlmVyw58t+ir65So3WEK Xrtw== X-Gm-Message-State: APt69E3lTD6VRaQKbDI32s9+6k4kbvWEngBkfDIopsdcUzRG91bQyM+P /JV2G0BaCFaTlJeVGHU7cCIfokiC9w== X-Google-Smtp-Source: ADUXVKLR2X1gg4YQJOx9op/hi1ilmoDRk5F1BnlWsGi1ZnPKd0R0DntIaZcv7splwOKyDkCEgeIOvQcWdQ== MIME-Version: 1.0 X-Received: by 2002:a1f:44e:: with SMTP id 75-v6mr11727726vke.121.1529594300030; Thu, 21 Jun 2018 08:18:20 -0700 (PDT) Date: Thu, 21 Jun 2018 17:18:09 +0200 Message-Id: <20180621151809.10921-1-jannh@google.com> X-Mailer: git-send-email 2.18.0.rc1.244.gcf134e6275-goog Subject: [PATCH v2] sg: mitigate read/write abuse From: Jann Horn To: Doug Gilbert , "James E.J. Bottomley" , "Martin K. Petersen" , linux-scsi@vger.kernel.org, Christoph Hellwig , Al Viro , jannh@google.com Cc: linux-kernel@vger.kernel.org, Jens Axboe , FUJITA Tomonori , kernel-hardening@lists.openwall.com, security@kernel.org, Benjamin Block X-Virus-Scanned: ClamAV using ClamSMTP As Al Viro noted in commit 128394eff343 ("sg_write()/bsg_write() is not fit to be called under KERNEL_DS"), sg improperly accesses userspace memory outside the provided buffer, permitting kernel memory corruption via splice(). But it doesn't just do it on ->write(), also on ->read(). As a band-aid, make sure that the ->read() and ->write() handlers can not be called in weird contexts (kernel context or credentials different from file opener), like for ib_safe_file_access(). If someone needs to use these interfaces from different security contexts, a new interface should be written that goes through the ->ioctl() handler. I've mostly copypasted ib_safe_file_access() over as sg_safe_file_access() because I couldn't find a good common header - please tell me if you know a better way. The duplicate pr_err_once() calls are so that each of them fires once; otherwise, this would probably have to be a macro. changed in v2: - remove the bsg parts per Christoph Hellwig's request Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: Signed-off-by: Jann Horn --- drivers/scsi/sg.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 53ae52dbff84..51b685192646 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -51,6 +51,7 @@ static int sg_version_num = 30536; /* 2 digits for each component */ #include #include #include +#include /* for sg_safe_file_access() */ #include "scsi.h" #include @@ -209,6 +210,23 @@ static void sg_device_destroy(struct kref *kref); sdev_prefix_printk(prefix, (sdp)->device, \ (sdp)->disk->disk_name, fmt, ##a) +/* + * The SCSI interfaces that use read() and write() as an asynchronous variant of + * ioctl(..., SG_IO, ...) are fundamentally unsafe, since there are lots of ways + * to trigger read() and write() calls from various contexts with elevated + * privileges. This can lead to kernel memory corruption (e.g. if these + * interfaces are called through splice()) and privilege escalation inside + * userspace (e.g. if a process with access to such a device passes a file + * descriptor to a SUID binary as stdin/stdout/stderr). + * + * This function provides protection for the legacy API by restricting the + * calling context. + */ +static inline bool sg_safe_file_access(struct file *filp) +{ + return filp->f_cred == current_cred() && !uaccess_kernel(); +} + static int sg_allow_access(struct file *filp, unsigned char *cmd) { struct sg_fd *sfp = filp->private_data; @@ -393,6 +411,12 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos) struct sg_header *old_hdr = NULL; int retval = 0; + if (!sg_safe_file_access(filp)) { + pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n", + __func__, task_tgid_vnr(current), current->comm); + return -EINVAL; + } + if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))) return -ENXIO; SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, @@ -581,8 +605,11 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) sg_io_hdr_t *hp; unsigned char cmnd[SG_MAX_CDB_SIZE]; - if (unlikely(uaccess_kernel())) + if (!sg_safe_file_access(filp)) { + pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n", + __func__, task_tgid_vnr(current), current->comm); return -EINVAL; + } if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))) return -ENXIO;