Re: [RFC PATCH 3/7] arm64: add check for comparison against tagged address

[Dan Carpenter <dan.carpenter@oracle.com>]

I'm sorry for losing track of this.  I actually applied all the patches
when you sent them.  They still cause that (harmless) compile warning
which I mentioned so I think about you when ever I do a build.  :P  But
otherwise I had completely erased this from my mind.

On Mon, Oct 07, 2019 at 04:49:24PM +0100, Andrew Murray wrote:
> > +static void match_assign(struct expression *expr)
> > +{
> > +	char *left_name;
> > +	struct symbol *left_sym;
> > +
> > +	left_name = expr_to_var_sym(expr->left, &left_sym);
> > +	if (!left_name || !left_sym)
> > +		return;
> > +
> > +	/*
> > +	 * Once we have spotted a symbol of interest (one that may hold
> > +	 * an untagged memory address), we keep track of any assignments
> > +	 * made, such that we can also treat the assigned symbol as something
> > +	 * of interest. This tracking is limited in scope to the function.
> > +	 */
> > +	if (expr_has_memory_addr(expr->right))
> > +		insert_symbol(symbols, left_name, left_name);
> > +}
> 
> The purpose of this is to work around issues presented in this type of code:
> 
> unsigned long start = vma->vm_start;
> if (start & ~PAGE_MASK)
>    ;
> 
> Smatch will detect that vm_start is an untagged address, however unless it can
> learn that start is also an untagged address then it won't pick up the potential
> bug with line 2 where we compare a tagged address with an untagged address. The
> current mplementation will track these simple assignments but the scope is
> limited only to the current function.
> 
> A potential solution is to give Smatch a flag for each symbol that indicates if a
> variable is untagged, tagged, or unknown. This flag should be propagated across
> assignments (and similar) and stored in the database (to allow for knowledge of
> the tagged flag across function calls). This may be similar to the existing
> support in Smatch for USER_DATA. Does this seem a good fit?
> 

First of all, if it works for you and you send me a patch I'm likely
going to apply it regardless of my personal feelings about the issue.
You're absolutely welcome to add any information you want to the
database.  Right now my DB is around 20GB but if it's 30GB that's also
fine.

I have the memory of a gnat.  Why did we not use smatch_bits.c for this?
One problem is that smatch_bits.c has no users and has never been
tested.  In theory what that gives you is:

   103  struct bit_info {
   104          unsigned long long set;
   105          unsigned long long possible;
   106  };

The ->set bits are definitely set and the ->possible bits are possibly
set.  It's quite a lot of work to do it properly across functions because
you need to mirror smatch_param_set.c, smatch_param_limit.c and
smatch_param_filter.c but for bits.  Then I'll hook it into
smatch_implied.c as well.  Right now it just passes the information to
the called function.

> > +/*
> > + * Identify expressions that contain memory addresses, in the future
> > + * we may use annotations on symbols or function parameters.
> > + */
> > +static bool expr_has_memory_addr(struct expression *expr)
> > +{
> > +	if (expr->type == EXPR_PREOP || expr->type == EXPR_POSTOP)
> > +		expr = strip_expr(expr->unop);
> > +
> > +	if (expr_has_untagged_member(expr))
> > +		return true;
> > +
> > +	if (expr_has_untagged_macro(expr))
> > +		return true;
> > +
> > +	if (expr_has_untagged_symbol(expr))
> > +		return true;
> 
> We hardcode symbols of interest that we consider to be untagged addresses. This
> provides good coverage but isn't very flexible. A better approach would be to
> annotate the kernel with address space tags, such as is the case for __user,
> __percpu, etc. Thus variables, struct members and function parameters could be
> annotated to indicate that they contain untagged addresses. Unfortunately:
> 
>  - At present it's not possible to determine a struct member's address space
>    from Smatch

I'm not sure how to get the address space for anything not just struct
members.  :(  I will investigate.

>  - It's not clear how you would annotate a macro such as PAGE_MASK (Smatch
>    already has difficulty spotting a macro inside a macro, e.g. PAGE_MASK
>    inside offset_in_page).
> 
> Is there a solution for this?
> 

I did some work exposing nested macros for this, right?  I was so sure
I sent an email about that...  :/  get_all_macros() and
get_inner_macro() for use in expr_has_untagged_macro().  Apparently I
didn't send that email.

regards,
dan carpenter