Re: [PATCH v2 1/3] checks: add phandle with arg property checks

On Fri, Aug 25, 2017 at 10:27:09AM -0500, Rob Herring wrote:

On Fri, Aug 25, 2017 at 8:17 AM, David Gibson
[off-list ref] wrote:

quoted

On Thu, Aug 24, 2017 at 02:19:27PM -0500, Rob Herring wrote:

quoted

On Thu, Aug 24, 2017 at 12:23 PM, Rob Herring [off-list ref] wrote:

quoted

On Wed, Aug 23, 2017 at 9:03 PM, David Gibson
[off-list ref] wrote:

quoted

On Tue, Aug 22, 2017 at 06:02:06PM -0500, Rob Herring wrote:

quoted

Many common bindings follow the same pattern of client properties
containing a phandle and N arg cells where N is defined in the provider
with a '#<specifier>-cells' property such as:

[...]

quoted

quoted

quoted

+             if (prop->val.len < ((cell + cellsize + 1) * sizeof(cell_t))) {
+                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+                          prop->name, prop->val.len, cellsize, node->fullpath);
+             }

How will this cope if the property is really badly formed - e.g. a 3
byte property, so you can't even grab a whole first phandle?  I think
it will trip the assert() in propval_cell_n() which isn't great.

At least for your example, we'd exit the loop (cell < 3/4). But I need
to a check for that because it would be silent currently. I'll add a
check that the size is a multiple of 4 and greater than 0.

However, the check here is not perfect because we could have
"<&phandle1 1 2>" when really it should be "<&phandle1 &phandle2
&phandle3>". We don't fail until we're checking the 2nd phandle.
That's why I added the "or bad phandle" and the cell # in the message
above. In the opposite case, we'd be silent. One thing that could be
done is double check things against the markers if they are present.

Here's what that looks like:

/* If we have markers, verify the current cell is a phandle */
if (prop->val.markers) {
  struct marker *m = prop->val.markers;
  for_each_marker_of_type(m, REF_PHANDLE) {
    if (m->offset == (cell * sizeof(cell_t)))
      break;
  }
  if (!m)
  FAIL(c, dti, "Property '%s', cell %d is not a valid phandle in %s",
    prop->name, cell, node->fullpath);

The logic seems sound, but I don't like the message.  An integer
literal is no less a phandle than a reference, just usually not the
best way of entering one.

Then what do you propose? There's not really any way I can distinguish
a mixture. If #whatever-cells was wrong and I'm pointing to an integer
literal that's not a phandle, it looks no different than if
#whatever-cells is correct and I'm pointing to an integer literal that
is a phandle.

Simply s/valid phandle/phandle reference/ would be sufficient.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson