smatch and copy_{to,from}_user return values

smatch and copy_{to,from}_user return values

[Rasmus Villemoes]

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

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

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


[-- Attachment #2: check_return_efault2.c --]
[-- Type: text/x-csrc, Size: 2096 bytes --]

/*
 * 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);
}

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

[Heiko Carstens]

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.]

This has been reported just a couple of days ago - fixes will go
upstream soon.

Thanks for reporting anyway!

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

[Dan Carpenter]

It turns out that my check for returning -EIO instead of -EFAULT doesn't
work at all...  :/  How the cross function DB works is that it tries to
figure out groups of states which should go together.  Most of the time
you could combine all the failure paths together and combine the success
paths together, for example.

But with copy_from_user() there is only one set of states recorded.

sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]|        INTERNAL | -1 |                      | ulong(*)(void*, void*, ulong) | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]|     CAPPED_DATA | -1 |                    $ |                      | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]| UNTRACKED_PARAM |  0 |                    $ |                      | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]| UNTRACKED_PARAM |  1 |                    $ |                      | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]|     PARAM_LIMIT |  2 |                    $ |             1-u64max | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]| NO_OVERFLOW_SIMPLE |  0 |                    $ |                      | 
sound/pci/rme9652/hdspm.c | copy_from_user | 1093 | 0-u32max[<=$2]|        STMT_CNT | -1 |                      |                   16 |

I could modify smatch_data/db/fixup_kernel.sh to hard code the desired
split, which is sort of awkward and also this in inlined so that makes
even more awkward.  Or I could create a new table with a manual way of
forcing splits in return states with entries like:

copy_from_user 0-u32max[<=$2] 0 1-u32max[<=$2]

That's probably the way to go, actually.

The check for propagating the return from copy_from_user() only looks
at assignments.  It sets the state to &remaining intialialy and then
if it sees a comparison with "if (ret) " it set the false path to &ok.
Then if we "return ret;" and "ret" is in state &remaining then complain.


static void match_copy(const char *fn, struct expression *expr, void *unused)
{
        if (expr->op == SPECIAL_SUB_ASSIGN)
                return;
        set_state_expr(my_id, expr->left, &remaining);
}

static void match_condition(struct expression *expr)
{
        if (!get_state_expr(my_id, expr))
                return;
        /* If the variable is zero that's ok */
        set_true_false_states_expr(my_id, expr, NULL, &ok);
}

regards,
dan carpenter

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

[Dan Carpenter]

This turned out way more complicated than I expected...  It's working
now.  Probably I will should filter out the function which return
true/false instead of negative error codes.  I was surprised it
generated so few warnings, which suggests that maybe there is a problem.
No real false positives though (one place had multiple errors so I guess
the EIO trumped the EFAULT).

I'm not sure if it's worth sending emails about these...  But I've added
a new --pedantic option for reviewing new code so this would definitely
qualify for that.

regards,
dan carpenter

kernel/trace/trace_uprobe.c:122 get_user_stack_nth() warn: return -EFAULT instead of '0'
arch/x86/kernel/uprobes.c:1070 arch_uretprobe_hijack_return_addr() warn: return -EFAULT instead of '(-1)'
arch/x86/kernel/fpu/signal.c:33 check_for_xstate() warn: return -EFAULT instead of '(-1)'
arch/x86/kvm/cpuid.c:955 sanity_check_entries() warn: return -EFAULT instead of '1'
fs/coda/pioctl.c:62 coda_pioctl() warn: return -EFAULT instead of '-EINVAL'
ipc/msgutil.c:157 store_msg() warn: return -EFAULT instead of '(-1)'
ipc/msgutil.c:164 store_msg() warn: return -EFAULT instead of '(-1)'
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:1966 r871x_get_ap_info() warn: return -EFAULT instead of '-EINVAL'
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2005 r871x_get_ap_info() warn: return -EFAULT instead of '-EINVAL'
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2020 r871x_set_pid() warn: return -EFAULT instead of '-EINVAL'
drivers/char/xillybus/xillybus_core.c:1340 xillybus_write() warn: return -EFAULT instead of '-EIO'
drivers/usb/mon/mon_bin.c:282 copy_from_buf() warn: return -EFAULT instead of '-EINVAL'
drivers/misc/bcm-vk/bcm_vk_dev.c:956 bcm_vk_load_image() warn: return -EFAULT instead of '-EACCES'
drivers/misc/sgi-gru/grukdump.c:30 gru_user_copy_handle() warn: return -EFAULT instead of '(-1)'
drivers/gpu/drm/amd/amdgpu/amdgpu_fw_attestation.c:108 amdgpu_fw_attestation_debugfs_read() warn: return -EFAULT instead of '-EINVAL'
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:126 amdgpu_ras_debugfs_read() warn: return -EFAULT instead of '-EINVAL'
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:173 amdgpu_ras_debugfs_ctrl_parse_data() warn: return -EFAULT instead of '-EINVAL'
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:215 amdgpu_ras_debugfs_ctrl_parse_data() warn: return -EFAULT instead of '-EINVAL'
drivers/scsi/lpfc/lpfc_debugfs.c:2427 lpfc_debugfs_dif_err_write() warn: return -EFAULT instead of '0'
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:3764 debugfs_bist_linkrate_v3_hw_write() warn: return -EFAULT instead of '-EINVAL'
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:3854 debugfs_bist_code_mode_v3_hw_write() warn: return -EFAULT instead of '-EOVERFLOW'
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:3983 debugfs_bist_mode_v3_hw_write() warn: return -EFAULT instead of '-EOVERFLOW'
drivers/crypto/qat/qat_common/adf_ctl_drv.c:100 adf_ctl_alloc_resources() warn: return -EFAULT instead of '-EIO'
drivers/media/pci/ttpci/av7110_hw.c:889 LoadBitmap() warn: return -EFAULT instead of '-EINVAL'
drivers/media/pci/ddbridge/ddbridge-core.c:631 ddb_output_write() warn: return -EFAULT instead of '-EIO'
security/tomoyo/common.c:225 tomoyo_flush() warn: return -EFAULT instead of '0'