handle resolve of NULL pointers

[andrewstrohman]

When a pointer to a BTF struct is NULL, every type of matchArgs selector should fail to match, because the argument was not really resolved.

When we fail to dereference during resolve, report the "depth" at which the failure occurred, and expose this in the event, so that users can distinguish between bogus and legitimate arg values.

When investigating how to plumb this to the event, I found a bug regarding how returnCopy works. I've added a fix for that as well.

[netlify[bot]]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	32525cb2b6832587fbabe5c7f31caed81c2e1c8b
🔍 Latest deploy log	https://app.netlify.com/projects/tetragon/deploys/6945ccce1e9481000885a29c
😎 Deploy Preview	https://deploy-preview-4327--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[andrewstrohman]

Thank you for taking time to write this PR. I opened an issue for this few month ago, see #3728.

Thanks for bringing this to my attention, and your feedback/ideas here.

• First, what happen if the null structure is not the last resolving field. How can you track where it ends ?

I've changed things so that I keep track of which level of depth we are unable to dereference. I use a sentinel value of -1 to indicated that no issues were detected. This is then used as an indicator that selectors against this arg shouldn't match. I'm thinking that perhaps this level of depth of failure information could be put in the event not only to distinguish resolve failures from success, but also what depth the failure occurred.

• Also, what happened if you are resolving int *value and the ptr is null ?

I'm investigated this, and it seems that this scenario doesn't work for the non-NULL case. So, I want to fix that first, before considering how to handle the NULL case. This is what I'm trying:

struct mystruct {
	uint8_t  v8;
	uint16_t v16;
	uint32_t v32;
	uint64_t v64;
	struct mysubstruct sub;
	struct mysubstruct *subp;
	uint32_t *v32p;
};

In the userspace program I'm testing against, I do this:

struct mystruct s = {0};
uint32_t value = 3;
s.v32p = &value;

The policy's arg config:

    - index: 1
      type: "uint32"
      btfType: "mystruct"
      resolve: "v32p"

Is this the scenario you're describing here?

It seems like we might need to do one more iteration of calling extract_arg_depth and dereference again to make this work as expected. Put another way, I think that we are acting on a pointer as if it's an integer.

• And if you are resolving a struct (e.g. type: file) you need to let extract_arg ends correctly, because at this point you have no idea if extract_arg return a legitimate 0 or a null struct. But when resolving it's fields, you will figure out that it is actually NULL.

I'll look into this more closely, but from reading the code it seems that we don't handle the type: file NULL case correctly, even outside of resolving. At first glance, I'm not seeing checks for probe_read() return values even though this type requires dereference. Overall, I thinking that my strategy is to fix issues in read_arg() where we should be watching the return value of probe_read() (types that require dereference), if they exist, independently from catching dereference problems in extract_arg_depth. I think the combination of these two methods will cover all bases. What do you think about this approach?

The potential solution I was thinking of for this was to take all the return values of the probe_read and find a way to send this error to the userspace if it is < 0.

Thanks for this idea. I think this is the right approach, and I have started moving in this direction.

So in case of a null pointer, you can have a new error code that the kernel does not cover, for instance -100, that is dedicated to it.

I'm not sure if I follow this. I think we need to communicate the error out of band of the arg value or else we will have a collision. Perhaps we can report the depth at which dereferencing failed in the event, in order to distinguish between resolve success and failure.

I'm trying to figure out what is the right behavior is for when we cannot resolve due to a NULL pointer. My current approach is to indicate there was a resolve error, but not let that prevent the event from firing, unless there is a matchArgs selector associated with the arg. In this method, I would communicate that the arg was not actually resolved in the event somehow. But, I realized from the issue that you created about this, that NULL string pointers prevent an event from firing, regardless of selectors. So maybe that's a more appropriate outcome.

[kkourt]

I'm trying to figure out what is the right behavior is for when we cannot resolve due to a NULL pointer. My current approach is to indicate there was a resolve error, but not let that prevent the event from firing, unless there is a matchArgs selector associated with the arg. In this method, I would communicate that the arg was not actually resolved in the event somehow.

Above makes sense to me.

But, I realized from the issue that you created about this, that NULL string pointers prevent an event from firing, regardless of selectors. So maybe that's a more appropriate outcome.

I don't think that the appropriate outcome.

I'll use the example from the issue for clarity:

  lsmhooks:
  - hook: "bprm_check_security"
    args:
    - index: 0
      type: "string"
      resolve: "executable.f_path.dentry.d_name.name"
    selectors:
    - matchActions:
      - action: Post

In this case (that is, where there no matchArgs), I think we should generate an event (perhaps with an empty argument, or even a special value to indicate that some resolution went wrong).

[andrewstrohman]

I'm converting back to draft, as I just realized a problem with my approach. I need to think about tracepoints and usdt a bit more.

[olsajiri]

hi, just some thoughts for discussion.. I dont feel strongly about it, I'm not sure what the right solution is ;-)

do we need that error-depth number in the final event? I wonder if you see it in event in most cases you'll know what failed.. in which case using some flag instead would be enough?

and if there's reason to keep it (I guess long derefs would justify that) could we maybe store the part of the deref chain (string) that failed instead of depth (we have that deref chain info already)

and how about using metric instead, something like deref_fails_cnt[policy,sensor,arg-idx,deref-chain] .. as I'm not sure this info should be part of the final event.. but I could be easily wrong

[andrewstrohman]

hi, just some thoughts for discussion.. I dont feel strongly about it, I'm not sure what the right solution is ;-)

Thanks for the review and helping me think this through.

do we need that error-depth number in the final event? I wonder if you see it in event in most cases you'll know what failed.. in which case using some flag instead would be enough?

I'd be OK with just a boolean flag that indicates that resolving failed. My primary goal here is to indicate that the argument value is bogus. I can't just remove the bogus arg value from the event because that would shift some args values to the left of what was configured in the spec, causing a mis-match with expectations.

I think I went down this path of recording which dereference failed, because of @tdaudi's earlier comment. He said:

First, what happen if the null structure is not the last resolving field. How can you track where it ends ?

@tdaudi, did I understand your comment correctly? Is this something that is important to you? If this is not what you meant, or you don't feel strongly about this, I think we should just go with a boolean flag. I initially thought this might be nice to expose, but it's not important to me.

If users really want to know where the NULL deference happened, they can resolve the intermediate pointers to see which one was NULL, if the they have the args to spare (we have a max of 5). This way, they can also run a selector against it too, whereas just reporting the depth does not allow them to filter based on it.

and if there's reason to keep it (I guess long derefs would justify that) could we maybe store the part of the deref chain (string) that failed instead of depth (we have that deref chain info already)

When you say "long derefs" here, I think you mean multiple dereferences per resolve. Please let me know if I misunderstood. I agree that if we decide to pinpoint the exact deference that failed it would be better to go all the way and show them where it failed via a prefix of the resolve configuration string. Just reporting the depth requires the user to know too much (and do extra investigation) in order to be able to interpret it in a useful way. But I'm currently leaning toward just changing it to be a boolean.

and how about using metric instead, something like deref_fails_cnt[policy,sensor,arg-idx,deref-chain] .. as I'm not sure this info should be part of the final event.. but I could be easily wrong

When you say "this info", I think you mean the depth of dereference failure here. I think we agree that we need some signal in the event that indicates that the arg value is bogus. Please correct me if I misunderstand your position here.

I would prefer your first or second second suggestion over this, because the user cannot definitely know which event the counter increment was related to.

When we fix read_arg(),so that NULL pointers won't prevent events(see here for background), we can use this same boolean flag to indicate the dereference failure, unifying the two sources of dereference failures.

[tdaudi]

@tdaudi, did I understand your comment correctly? Is this something that is important to you? If this is not what you meant, or you don't feel strongly about this, I think we should just go with a boolean flag. I initially thought this might be nice to expose, but it's not important to me.

If users really want to know where the NULL deference happened, they can resolve the intermediate pointers to see which one was NULL, if the they have the args to spare (we have a max of 5). This way, they can also run a selector against it too, whereas just reporting the depth does not allow them to filter based on it.

My initial idea was to have a convinient way to know which field in the resolve path is null to avoid spending time on updating the policy and testing again. But for production, I think it is interesting to know where the resolve fails when the null value is unexpected. Knowing where the null appears help understanding the reason and avoid the difficulty of having to reproduce it, especially if it occurs rarely.