Re: smatch and copy_{to,from}_user return values

Dan Carpenter <[email protected]> Wed, 3 Mar 2021 14:20:46 +0300
Newsgroups org.kernel.vger.smatch,org.kernel.vger.linux-s390
Message-ID <20210303112046.GB2222@kadam> 
On Wed, Mar 03, 2021 at 08:50:19AM +0100, Rasmus Villemoes wrote:
> Hi Dan
> 
> If you look at vfio_ccw_mdev_ioctl() in drivers/s390/cio/vfio_ccw_ops.c,
> and vfio_ap_mdev_get_device_info() in drivers/s390/crypto/vfio_ap_ops.c,
> there are examples of functions that can both return -Esomething as well
> as may return the return value of a copy_{to,from}_user directly (i.e.,
> in case of error some positive number).
> 
> [Those "return copy_to_user();" should probably all be changed to
> "return copy_to_user() ? -EFAULT : 0;" - cc'ing the s390 list in case
> the maintainers want to do that.]
> 
> Can smatch detect such cases? I seem to recall it has some concept of
> tagging a function as "returning -Efoo or 0", so it would also need to
> know that copy_{to,from}_user does not return -Efoo. And it also needs
> to follow the control flow, so
> 
>  ret = copy_to_user();
>  if (ret)
>     return -EIO;
>  something_else;
>  return ret; /* this is 0 */
> 
> doesn't trigger. And there's gonna be some false positives around signal
> frame setup, which do a lot of "err |= foo(); err |= bar()" where foo()
> report errors as -Exxx and bar can be a copy_to_user(), but in the end
> err is only checked against 0.
> 
> Rasmus

Yeah.  There is already a check for if you propagate the return from
copy_from_user()...  The problem is that this is s390 code and I don't
have a cross compiler set up so this was never reported or fixed.

When I first saw your email, I didn't read it carefully and I thought
you were complaining about code that returns -EIO where -EFAULT is
intended.  Anyway, I wrote that check before re-reading the email.  LOL.
Attached.

I did a quick "git grep |= copy_" and I see that's mostly used in
signal code where the caller doesn't care about the error code, only
whether it's zero vs non-zero.  I considered about excluding "arch/"
from the check but then there are only two instances where this is used
and both are correct.

regards,
dan carpenter
check_return_efault2.c (text/x-csrc, 2 KB) 
/*
 * Copyright (C) 2021 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static const sval_t ulong_one	= { .type = &ulong_ctype, .value = 1 };
static const sval_t ulong_INT_MAX = { .type = &ulong_ctype, .value = INT_MAX };

STATE(copy_failed);

static void match_copy_failed(const char *fn, struct expression *call_expr,
			      struct expression *expr, void *_unused)
{
	set_state(my_id, "path", NULL, &copy_failed);
}

static void match_return(struct expression *expr)
{
	char *macro = NULL;
	sval_t ret;

	if (!get_value(expr, &ret))
		return;
	if (ret.value == -14)
		return;
	if (get_state(my_id, "path", NULL) != &copy_failed)
		return;

	if (expr->type == EXPR_PREOP && expr->op == '-')
		macro = get_macro_name(expr->unop->pos);

	if (macro)
		sm_warning("return -EFAULT instead of '-%s'", macro);
	else
		sm_warning("return -EFAULT instead of '%s'", sval_to_str(ret));
}

void check_return_efault2(int id)
{
	if (option_project != PROJ_KERNEL)
		return;

	my_id = id;

	return_implies_state_sval("copy_to_user", ulong_one, ulong_INT_MAX, &match_copy_failed, NULL);
	return_implies_state_sval("copy_from_user", ulong_one, ulong_INT_MAX, &match_copy_failed, NULL);
	return_implies_state_sval("__copy_to_user", ulong_one, ulong_INT_MAX, &match_copy_failed, NULL);
	return_implies_state_sval("__copy_from_user", ulong_one, ulong_INT_MAX, &match_copy_failed, NULL);

	add_hook(&match_return, RETURN_HOOK);
}