From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1031516AbdAJTbB (ORCPT <rfc822;w@1wt.eu>);
        Tue, 10 Jan 2017 14:31:01 -0500
Received: from mail2-relais-roc.national.inria.fr ([192.134.164.83]:21634 "EHLO
        mail2-relais-roc.national.inria.fr" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S942790AbdAJTad (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 10 Jan 2017 14:30:33 -0500
X-IronPort-AV: E=Sophos;i="5.33,344,1477954800"; 
   d="scan'208";a="254742306"
Date: Tue, 10 Jan 2017 20:30:29 +0100 (CET)
From: Julia Lawall <julia.lawall@lip6.fr>
X-X-Sender: jll@hadrien
To: Kees Cook <keescook@chromium.org>
cc: cocci@systeme.lip6.fr, Pengfei Wang <wpengfeinudt@gmail.com>,
        Vaishali Thakkar <vthakkar1994@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] coccicheck: add a test for repeat memory fetches
In-Reply-To: <CAGXu5j+Nk9r7K=FJe4v5b8mX_Fn411JMGks2sueyOFS2aTo1Ow@mail.gmail.com>
Message-ID: <alpine.DEB.2.20.1701102028300.1981@hadrien>
References: <20170109231323.GA89642@beast> <alpine.DEB.2.20.1701101927070.1981@hadrien> <CAGXu5j+Nk9r7K=FJe4v5b8mX_Fn411JMGks2sueyOFS2aTo1Ow@mail.gmail.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On Tue, 10 Jan 2017, Kees Cook wrote:

> On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439
> >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491
> >
> > Do you want the above results?  They have the form:
> >
> > if (copy_from_user(&t, useraddr, sizeof(t)))
> >
> > My reasoning was that there could be no problem here, because the size is
> > the size of the destination structure.  It doesn't depend on user level data.
>
> They're likely false positives, but it does follow the pattern of
> reading the same userspace location twice:
>
>         if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
>                 return -EFAULT;
>
>         switch (cmd) {
>         case CHELSIO_SET_QSET_PARAMS:{
>                 int i;
>                 struct qset_params *q;
>                 struct ch_qset_params t;
>                 int q1 = pi->first_qset;
>                 int nqsets = pi->nqsets;
>
>                 if (!capable(CAP_NET_ADMIN))
>                         return -EPERM;
>                 if (copy_from_user(&t, useraddr, sizeof(t)))
>                         return -EFAULT;
>
> If there is any logic that examines cmd (u32) and operates on t
> (struct ch_qset_params), there could be a flaw. It doesn't look like
> it here, but a "correct" version of this would be:
>
>                 if (copy_from_user(&t, useraddr, sizeof(t)))
>                         return -EFAULT;
>                 t.cmd = cmd;

OK, I'm fine with putting them all back.

For another issue, what about code like the following:

        if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
                return -EFAULT;

        if ((u_cmd.outsize > EC_MAX_MSG_BYTES) ||
	    (u_cmd.insize > EC_MAX_MSG_BYTES))
		return -EINVAL;

        s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize),
                        GFP_KERNEL);
        if (!s_cmd)
                return -ENOMEM;

        if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) {
		ret = -EFAULT;
                goto exit;
        }

It doesn't actually test sizeof(*s_cmd) + u_cmd.outsize, but it does test
u_cmd.outsize > EC_MAX_MSG_BYTES, and presumably that test accounts for
the extra sizeof(*s_cmd) value.

julia