detecting misuse of of_get_property

detecting misuse of of_get_property

[Rasmus Villemoes]

Hi,

I just spent some time trying to convert some so far PPC-only drivers to
be more generic. One of the things I had to do was convert stuff like

  u32 *val = of_get_property(np, "bla", NULL);
  do_stuff_with(*val);

with

  of_property_read_u32(np, "bla", &val);
  do_stuff_with(val);

(error checking omitted for simplicity). The problem is that
of_get_property() just returns void*. When the property is just a
string, there's no problem interpreting that as a char*. But when the
property is a number of array of numbers, I'd like some way to flag
casting it to u32* as an error - if you cast it to a (pointer to integer
type wider than char), it must be to a __be32*. Is there some way
sparse/smatch could help find such cases?

Rasmus

Re: detecting misuse of of_get_property

[Luc Van Oostenryck]

On Mon, Oct 28, 2019 at 08:32:42PM +0100, Rasmus Villemoes wrote:
> Hi,
> 
> I just spent some time trying to convert some so far PPC-only drivers to
> be more generic. One of the things I had to do was convert stuff like
> 
>   u32 *val = of_get_property(np, "bla", NULL);
>   do_stuff_with(*val);
> 
> with
> 
>   of_property_read_u32(np, "bla", &val);
>   do_stuff_with(val);
> 
> (error checking omitted for simplicity). The problem is that
> of_get_property() just returns void*. When the property is just a
> string, there's no problem interpreting that as a char*. But when the
> property is a number of array of numbers, I'd like some way to flag
> casting it to u32* as an error - if you cast it to a (pointer to integer
> type wider than char), it must be to a __be32*. Is there some way
> sparse/smatch could help find such cases?

If I understand you correctly, you would need a kind of 'soft'
bitwise pointer?
I guess it shouldn't be too hard to add a new flag which would
allow cast of bitwise pointers to pointers to char/void (see
at end of evaluate.c:evaluate_cast()).

Note: casts from bitwise pointer to void* are already allowed.

-- Luc

Re: detecting misuse of of_get_property

[Rasmus Villemoes]

On 28/10/2019 23.49, Luc Van Oostenryck wrote:
> On Mon, Oct 28, 2019 at 08:32:42PM +0100, Rasmus Villemoes wrote:
>> Hi,
>>
>> I just spent some time trying to convert some so far PPC-only drivers to
>> be more generic. One of the things I had to do was convert stuff like
>>
>>   u32 *val = of_get_property(np, "bla", NULL);
>>   do_stuff_with(*val);
>>
>> with
>>
>>   of_property_read_u32(np, "bla", &val);
>>   do_stuff_with(val);
>>
>> (error checking omitted for simplicity). The problem is that
>> of_get_property() just returns void*. When the property is just a
>> string, there's no problem interpreting that as a char*. But when the
>> property is a number of array of numbers, I'd like some way to flag
>> casting it to u32* as an error - if you cast it to a (pointer to integer
>> type wider than char), it must be to a __be32*. Is there some way
>> sparse/smatch could help find such cases?
> 
> If I understand you correctly, you would need a kind of 'soft'
> bitwise pointer?

Yes, that's a very good way of putting it.

> I guess it shouldn't be too hard to add a new flag which would
> allow cast of bitwise pointers to pointers to char/void (see
> at end of evaluate.c:evaluate_cast()).

Hm, yeah, but it should also allow casting to __be32* , but not u32* or
__le32* (though somebody must have gone out of their way to introduce a
bug in the latter case). Don't spend too much time on it, I was just
wondering if there was an easy (maybe already existing) way.

Thanks,
Rasmus

Re: detecting misuse of of_get_property

[Dan Carpenter]

This should probably work?

regards,
dan carpenter

diff --git a/check_list.h b/check_list.h
index 564bd1c2..5fa9a269 100644
--- a/check_list.h
+++ b/check_list.h
@@ -191,6 +191,7 @@ CK(check_nospec_barrier)
 CK(check_spectre)
 CK(check_spectre_second_half)
 CK(check_implicit_dependencies)
+CK(check_of_get_property)
 
 /* wine specific stuff */
 CK(check_wine_filehandles)
diff --git a/check_of_get_property.c b/check_of_get_property.c
new file mode 100644
index 00000000..b7aa8824
--- /dev/null
+++ b/check_of_get_property.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2019 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_slist.h"
+
+static int my_id;
+
+static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
+{
+	struct expression *left = expr->left;
+	struct symbol *type;
+
+	type = get_type(left);
+	if (!type || type->type != SYM_PTR)
+		return;
+	type = get_base_type(type);
+	if (type_bits(type) == 8)
+		return;
+	if (type->type == SYM_RESTRICT)
+		return;
+	sm_warning("'%s' returns big endian data", fn);
+}
+
+void check_of_get_property(int id)
+{
+	my_id = id;
+
+	if (option_project != PROJ_KERNEL)
+		return;
+
+	add_function_assign_hook("of_get_property", &match_of_get_property, NULL);
+}

Re: detecting misuse of of_get_property

[Rasmus Villemoes]

On 29/10/2019 11.50, Dan Carpenter wrote:
> This should probably work?

I haven't tested it, but yes, something like that. Can you also do the
case of struct property::value, i.e. handling

  struct property *p = ...;
  u32 *v = p->value;

Thanks,
Rasmus

Re: detecting misuse of of_get_property

[Dan Carpenter]

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

On Tue, Oct 29, 2019 at 12:43:24PM +0100, Rasmus Villemoes wrote:
> On 29/10/2019 11.50, Dan Carpenter wrote:
> > This should probably work?
> 
> I haven't tested it, but yes, something like that. Can you also do the
> case of struct property::value, i.e. handling
> 
>   struct property *p = ...;
>   u32 *v = p->value;

Attached.

This has a bunch of flaws like "void *value = p->value;" doesn't
generate a warning.  A function that returns a u32 pointer doing
"return p->value;" doesn't generate a value.

Anyway, it's a starting point to experiment with.

regards,
dan carpenter


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

/*
 * Copyright (C) 2019 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_slist.h"

static int my_id;

static bool is_u8_or_endian(struct expression *expr)
{
	struct symbol *type;

	type = get_type(expr);
	if (!type || type->type != SYM_PTR)
		return true;
	type = get_base_type(type);
	if (type == &void_ctype)
		return true;
	if (type_bits(type) == 8)
		return true;
	if (type->type == SYM_RESTRICT)
		return true;
	return false;
}

static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
{
	if (is_u8_or_endian(expr->left))
		return;
	sm_warning("'%s' returns big endian data", fn);
}

static void match_assign(struct expression *expr)
{
	struct expression *right;
	struct symbol *type;

	if (expr->op != '=')
		return;
	if (is_u8_or_endian(expr->left))
		return;

	right = strip_expr(expr->right);
	if (!is_void_pointer(right))
		return;
	if (right->type != EXPR_DEREF)
		return;
	if (!right->member || strcmp(right->member->name, "value") != 0)
		return;

	type = get_type(right->deref);
	if (!type || type->type != SYM_STRUCT || !type->ident ||
	    strcmp(type->ident->name, "property") != 0)
		return;

	sm_warning("'(struct property)->value' is big endian");
}

void check_of_get_property(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_assign_hook("of_get_property", &match_of_get_property, NULL);
	add_hook(&match_assign, ASSIGNMENT_HOOK);
}

Re: detecting misuse of of_get_property

[Luc Van Oostenryck]

On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> +{
> +	struct expression *left = expr->left;
> +	struct symbol *type;
> +
> +	type = get_type(left);
> +	if (!type || type->type != SYM_PTR)
> +		return;
> +	type = get_base_type(type);
> +	if (type_bits(type) == 8)
> +		return;
> +	if (type->type == SYM_RESTRICT)
> +		return;

Wouldn't this also silently accept assignments to any bitwise
type: __le32, __be16, ... ? 

-- Luc

Re: detecting misuse of of_get_property

[Dan Carpenter]

On Tue, Oct 29, 2019 at 12:47:50PM +0100, Luc Van Oostenryck wrote:
> On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> > +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> > +{
> > +	struct expression *left = expr->left;
> > +	struct symbol *type;
> > +
> > +	type = get_type(left);
> > +	if (!type || type->type != SYM_PTR)
> > +		return;
> > +	type = get_base_type(type);
> > +	if (type_bits(type) == 8)
> > +		return;
> > +	if (type->type == SYM_RESTRICT)
> > +		return;
> 
> Wouldn't this also silently accept assignments to any bitwise
> type: __le32, __be16, ... ? 

It does, yes.  I'm not sure how big of an issue that is...  I always
just throw a check together and test it before I decide if it's worth
investing more time into it.

regards,
dan carpenter

Re: detecting misuse of of_get_property

[Luc Van Oostenryck]

On Tue, Oct 29, 2019 at 03:55:19PM +0300, Dan Carpenter wrote:
> On Tue, Oct 29, 2019 at 12:47:50PM +0100, Luc Van Oostenryck wrote:
> > On Tue, Oct 29, 2019 at 01:50:58PM +0300, Dan Carpenter wrote:
> > > +static void match_of_get_property(const char *fn, struct expression *expr, void *unused)
> > > +{
> > > +	struct expression *left = expr->left;
> > > +	struct symbol *type;
> > > +
> > > +	type = get_type(left);
> > > +	if (!type || type->type != SYM_PTR)
> > > +		return;
> > > +	type = get_base_type(type);
> > > +	if (type_bits(type) == 8)
> > > +		return;
> > > +	if (type->type == SYM_RESTRICT)
> > > +		return;
> > 
> > Wouldn't this also silently accept assignments to any bitwise
> > type: __le32, __be16, ... ? 
> 
> It does, yes.  I'm not sure how big of an issue that is...

Probably not much if it's just a one shot for Rasmus and
probably not much more otherwise.

> I always
> just throw a check together and test it before I decide if it's worth
> investing more time into it.

Sure, but I was thinking about false negatives here.

Regards,
-- Luc