From patchwork Thu Dec 20 19:59:31 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tycho Andersen X-Patchwork-Id: 10739527 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8E9076C5 for ; Thu, 20 Dec 2018 19:59:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7E2EA28BA7 for ; Thu, 20 Dec 2018 19:59:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 725D828BBA; Thu, 20 Dec 2018 19:59:48 +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=-7.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham 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 0E87428BA7 for ; Thu, 20 Dec 2018 19:59:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731960AbeLTT7r (ORCPT ); Thu, 20 Dec 2018 14:59:47 -0500 Received: from mail-it1-f193.google.com ([209.85.166.193]:54873 "EHLO mail-it1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725372AbeLTT7r (ORCPT ); Thu, 20 Dec 2018 14:59:47 -0500 Received: by mail-it1-f193.google.com with SMTP id i145so3787018ita.4 for ; Thu, 20 Dec 2018 11:59:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tycho-ws.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=8vSgLE0139Uw28lgL88xITvwXqU1EcObnWsuEHGD+5Y=; b=s9T6xbpzYClqxDvUWuehyBSgKZ7K6qBkBvo98Qs5/uEhnJvMC3O14O9cWwuI6ZS1sK j/2zom16LM/I+Ct0E+9ZM1Q18t/zA4fDBKWUIOAKpJgyNU3nuFiI72H8V3RyxEK5lj+I FHOxfHiBDHhBWYit7vy4trGYT2vad67vOtdVarfufckFN0h04nyS1SF3X1DI5wW9AgEV FK80qGc5kh6uKjgPw9U+dUKAPa8bMHqxmTu5mZKQ5dZ13+zFZvLyIgNzpBZpbb79S8yi PSs93puzVzCNRgYrwWOFF687Vvu15lRG4RXpN7A33zfW439Y+3LC7p9ZPoMlZ4o1fwlq xGDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=8vSgLE0139Uw28lgL88xITvwXqU1EcObnWsuEHGD+5Y=; b=jELqFY1fjXPd65+IGr2jQxYN3QmaP7DSSk8QmgWO+oxKXN3X5cJo7rsrHGDyPE518K rDsHXz4QmcAbkkZx+t6jQ2kAmYQs7FI8yR4Rqa8oI/Iyaidcsy0G0C5sz1arvSP5zz0h hHdftFVZq1cfxN2CPtmA9nnaBelYgoMA6NDEqP4hC7xzuapnr3fOOlZt0qvcFDe2c7xp icWnYHxLDl2lJdc19tHHc+gIq5atmuM3JJS3P9qQf4Te7b0jehshzIxDNDTRJq+ThZP9 GeYc++Y1UrpU7bQSZUiHADYRZNYa8Xsow3fZhgwAzOoS0ziQN3IzY5UnXAI5u6lTjiy6 JgWA== X-Gm-Message-State: AA+aEWbWSpmYVevbYYTcEw2yXFa9ao2pyNogItWU9DEj/Wf5T3PgbkjB 3npyX7ykVomDJBR7TrjVQR4vbY1spiRz7Q== X-Google-Smtp-Source: ALg8bN5gsxi7ln4LRcWe3zrN7ql/+YYwVfsLywhwUqjMi63YIibJ0+Bf/vZNJ4t/bEmzH4Jl5TQnZA== X-Received: by 2002:a05:660c:74b:: with SMTP id a11mr32937itl.27.1545335985751; Thu, 20 Dec 2018 11:59:45 -0800 (PST) Received: from cisco.lan (71-218-133-134.hlrn.qwest.net. [71.218.133.134]) by smtp.gmail.com with ESMTPSA id b140sm5068429itc.4.2018.12.20.11.59.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 20 Dec 2018 11:59:44 -0800 (PST) From: Tycho Andersen To: linux-sparse@vger.kernel.org, kernel-hardening@lists.openwall.com Cc: Tycho Andersen Subject: [RFC v1 4/4] check copy_to_user() sizes Date: Thu, 20 Dec 2018 12:59:31 -0700 Message-Id: <20181220195931.20331-5-tycho@tycho.ws> X-Mailer: git-send-email 2.19.1 In-Reply-To: <20181220195931.20331-1-tycho@tycho.ws> References: <20181220195931.20331-1-tycho@tycho.ws> MIME-Version: 1.0 Sender: linux-sparse-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sparse@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The intent here is to be a smarter version of the size checking that's just a hard coded constant. The insight is that we know the type of the thing that's getting passed to copy_to_user(), so if we can evaluate the size, it should be <= sizeof(*the thing being copied). I've run this over x86 with allyes config, and it didn't find anything, so perhaps it isn't particularly useful. Indeed, most copy_to_user()s look like: copy_to_user(p, &foo, sizeof(foo)) so perhaps this isn't a surprise. The one potentially interesting case is the hard coded array length case: it's possible someone might change one place but not the other. See e.g. the first copy_to_user() in drivers/ata/libata-scsi.c, which looks like: copy_to_user(dst, dev->id, ATA_ID_WORDS * sizeof(u16)) id here is a u16 id[ATA_ID_WORDS], so it would be possible to change this in one place or not the other. Unfortunately, sparse seems to unify the type of dev->id to a u16* instead of u16[], so we can't see through this. Anyway, there are currently no violations of this in the kernel either, although it does emit some false positives like, fs/xfs/xfs_ioctl.c:1813:25: warning: copy_to_user() where size (13) is larger than src (1) due to the above. I wrote the code, so perhaps someday it will be useful to someone, so I'm posting it :) Signed-off-by: Tycho Andersen --- sparse.c | 45 ++++++++++++++++++++++ validation/copy_to_user_sizes.c | 53 ++++++++++++++++++++++++++ validation/copy_to_user_sizes_inline.c | 29 ++++++++++++++ 3 files changed, 127 insertions(+) diff --git a/sparse.c b/sparse.c index 8bcbe16..eb7d5d7 100644 --- a/sparse.c +++ b/sparse.c @@ -31,6 +31,7 @@ #include #include #include +#include #include "lib.h" #include "allocate.h" @@ -259,6 +260,49 @@ static void check_no_kernel_pointers(struct position pos, struct expression_list check_ptr_in_other_as(pos, base, 1); } +static void check_copy_size(struct position pos, struct expression_list *args) +{ + struct expression *src = ptr_list_nth_entry((struct ptr_list *)args, 1); + struct expression *size = ptr_list_nth_entry((struct ptr_list *)args, 2); + long long src_actual = LLONG_MAX; + long long size_actual; + struct symbol *base, *deref; + + /* get the type of src */ + base = resolve_arg_type(pos, src); + + /* and deref it to *src */ + deref = base->ctype.base_type; + + /* + * Note, this is never hit right now because sparse seems to unify + * arrays that are arguments to type *. + */ + if (is_array_type(base)) { + long long array_size = get_expression_value_silent(base->array_size); + printf("%s is array type\n", show_ident(base->ident)); + + /* failed to evaluate */ + if (array_size == 0) + return; + src_actual = array_size * bits_to_bytes(deref->bit_size); + } else { + src_actual = bits_to_bytes(deref->bit_size); + } + + /* Evaluate size, if we can */ + size_actual = get_expression_value_silent(size); + /* + * size_actual == 0 means that get_expression_value failed; of + * course we'll miss something if there is a 0 length copy, but + * then nothing will leak anyway so... + */ + if (size_actual == 0) + return; + + if (size_actual > src_actual) + warning(pos, "copy_to_user() where size (%lld) is larger than src (%lld)", size_actual, src_actual); +} #define check_memcpy check_memset #define check_cfu check_memset @@ -267,6 +311,7 @@ void check_ctu(struct position pos, struct expression_list *args) { check_memset(pos, args); check_no_kernel_pointers(pos, args); + check_copy_size(pos, args); } struct checkfn { diff --git a/validation/copy_to_user_sizes.c b/validation/copy_to_user_sizes.c new file mode 100644 index 0000000..f1f7b8e --- /dev/null +++ b/validation/copy_to_user_sizes.c @@ -0,0 +1,53 @@ +static void copy_to_user(void *to, const void *from, unsigned long n) +{ +} + +struct foo { + int bar; +}; + +static void main(void) +{ + void *p; + struct foo f; + int uninitialized; + + copy_to_user(p, &f, sizeof(f)); + copy_to_user(p, &f, sizeof(f)-1); + copy_to_user(p, &f, sizeof(f)+1); + copy_to_user(p, &f, 1); + copy_to_user(p, &f, 100); + copy_to_user(p, &f, uninitialized); +} + +#if 0 +static void broken(void) +{ + void *p; + char arr[100]; + struct foo foo_arr[2]; + + /* + * These all fail right now, because sparse seems to unify the type of + * arr/foo_arr to char * /struct foo *, instead of char[]/struct foo[]. + * + * That's unfortunate, because it means that these generate false + * positives in the kernel when using a static array length, and that + * seems like a case where this type of check would be especially + * useful. + */ + copy_to_user(p, arr, 100); + copy_to_user(p, arr, 101); + copy_to_user(p, foo_arr, sizeof(foo_arr)); + copy_to_user(p, foo_arr, sizeof(foo_arr)-1); + copy_to_user(p, foo_arr, sizeof(foo_arr[0])*2); +} +#endif +/* + * check-name: copy_to_user sizes + * + * check-error-start +copy_to_user_sizes.c:17:9: warning: copy_to_user() where size (5) is larger than src (4) +copy_to_user_sizes.c:19:9: warning: copy_to_user() where size (100) is larger than src (4) + * check-error-end + */ diff --git a/validation/copy_to_user_sizes_inline.c b/validation/copy_to_user_sizes_inline.c new file mode 100644 index 0000000..bd3c3bd --- /dev/null +++ b/validation/copy_to_user_sizes_inline.c @@ -0,0 +1,29 @@ +static inline void copy_to_user(void *to, const void *from, unsigned long n) +{ +} + +struct foo { + int bar; +}; + +static void main(void) +{ + void *p; + struct foo f; + int uninitialized; + + copy_to_user(p, &f, sizeof(f)); + copy_to_user(p, &f, sizeof(f)-1); + copy_to_user(p, &f, sizeof(f)+1); + copy_to_user(p, &f, 1); + copy_to_user(p, &f, 100); + copy_to_user(p, &f, uninitialized); +} +/* + * check-name: copy_to_user sizes + * + * check-error-start +copy_to_user_sizes_inline.c:17:21: warning: copy_to_user() where size (5) is larger than src (4) +copy_to_user_sizes_inline.c:19:21: warning: copy_to_user() where size (100) is larger than src (4) + * check-error-end + */