On Thu, Mar 10, 2022 at 04:30:42PM -0500, Oleg Drokin wrote:
>
> Thanks, I added this and there’s still no detection.
> Example code:
>
> __page = alloc_page(GFP_KERNEL);
> if (__page == NULL)
> return -ENOMEM;
>
> req = cfs_crypto_hash_init(cfs_alg, NULL, 0);
> if (IS_ERR(req)) {
> CERROR("%s: unable to initialize checksum hash %s\n",
> tgt_name(tgt), cfs_crypto_hash_name(cfs_alg));
> return PTR_ERR(req);
> }
>
> buffer = kmap(__page);
>
>
> diff --git a/check_unwind.c b/check_unwind.c
> index 569792ad..7611f377 100644
> --- a/check_unwind.c
> +++ b/check_unwind.c
> @@ -92,6 +92,11 @@ static struct ref_func_info func_table[] = {
>
> { "ieee80211_alloc_hw", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> { "ieee80211_free_hw", RELEASE, 0, "$" },
> + { "alloc_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> + { "alloc_page", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> + { "__get_free_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
> + { "free_pages", RELEASE, 0, "$" },
> + { "__free_page", RELEASE, 0, "$" },
> };
Hey Oleg,
Sorry, I should have tested this better...
There are two issues. The easy issue is that apparently I didn't stop
to consider error pointers as a failure return.
The more complicated problem is that the allocation hook will only
trigger for cases where Smatch knows what the return is. Right now
Smatch knows that alloc_pages either returns NULL, or non-NULL (but
potentially error pointers). This is return_implies_param_key_exact()
vs return_implies_param_key().
Of course, I changed it to _exact() deliberately to silence false
positives. I don't remember the details so I'll retest tonight and look
again on Monday.
An easy "fix" would be to add "alloc_pages 1-u64max 4096-ptr_max" so it
gets hardcoded on the next db rebuild, but that's kind of a hack as
well.
diff --git a/check_unwind.c b/check_unwind.c
index 569792ad5a57..c3015cca3374 100644
--- a/check_unwind.c
+++ b/check_unwind.c
@@ -92,6 +92,10 @@ static struct ref_func_info func_table[] = {
{ "ieee80211_alloc_hw", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
{ "ieee80211_free_hw", RELEASE, 0, "$" },
+
+ { "alloc_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+ { "__get_free_pages", ALLOC, -1, "$", &valid_ptr_min_sval, &valid_ptr_max_sval },
+ { "free_pages", RELEASE, 0, "$" },
};
static struct smatch_state *unmatched_state(struct sm_state *sm)
@@ -237,11 +241,14 @@ enum {
static int success_fail_positive(struct range_list *rl)
{
if (!rl)
- return UNKNOWN;
+ return SUCCESS;
if (sval_is_negative(rl_min(rl)) && sval_is_negative(rl_max(rl)))
return FAIL;
+ if (type_is_ptr(rl_type(rl)) && is_err_or_null(rl))
+ return FAIL;
+
if (rl_min(rl).value == 0)
return SUCCESS;
@@ -334,7 +341,7 @@ void check_unwind(int id)
if (info->call_back) {
add_function_hook(info->name, info->call_back, info);
} else if (info->implies_start && info->type == ALLOC) {
- return_implies_param_key_exact(info->name,
+ return_implies_param_key(info->name,
*info->implies_start,
*info->implies_end,
&return_param_alloc,
