From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:57268 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229718AbhCEKOw (ORCPT ); Fri, 5 Mar 2021 05:14:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=corp-2020-01-29; bh=yelKnsHpxqV3KJ4nsuvTDUoBJHixZnZ8Hw5llBdWxf0=; b=RkxlwgXrr1WelePW7vV8XPCFiT+9d4a4A7mp28lxk0lV7FRhM/4znJc6vQJmahnP1f32 NcN5BjZWkeIJYwjia4gjCI87Ca2+PcBDtFJE/y2wRsOcPjTHtuPvTcjYhzIotKHnMRtc FIZq887myvp47exUvJE08sRUiN1oJlr41kDPjckKvP4DStbqjc1gLdRJuV7XK+6RNkIY CGB6HluHT/1IW/9HQWxPg8yqUXzkegfec8+9oHQTCTwFQUDTuNlu9cgqtgLLtMd8s0O2 AwdvW7jCQtOAKWrfPrGbC3cjj4zSpzIajiPAp58h8AV/nCAJ9rqIyRDldG9e+24UeXRb BA== Date: Fri, 5 Mar 2021 13:14:34 +0300 From: Dan Carpenter Subject: Re: smatch and copy_{to,from}_user return values Message-ID: <20210305101434.GI2222@kadam> References: <20210303112046.GB2222@kadam> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210303112046.GB2222@kadam> List-ID: To: Rasmus Villemoes Cc: linux-s390@vger.kernel.org, smatch@vger.kernel.org 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