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.

[tdaudi]

Hello !

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

I don’t know of you will share my point of view because will I have thinking about this i have found 3 problems :

• First, what happen if the null structure is not the last resolving field. How can you track where it ends ?
• Also, what happened if you are resolving int *value and the ptr is null ?
• 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.

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. 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.

[tdaudi]

If the last btf_config resolve in a null pointer, you would not reach that code right ?

[andrewstrohman]

Yes, this is good point. I think my new placement for error detection takes care of this problem.

[tdaudi]

I think 2 test cases should be added. This one is good, but if sub alone is resolved, does it raise an error too ? For instance, if type: file is null, does Tetragon display null as well ?

[andrewstrohman]

I think 2 test cases should be added.

You go on to describe what sounds like one test case. What is the other test case you're thinking about here?

but if sub alone is resolved, does it raise an error too ? For instance, if type: file is null, does Tetragon display null as well ?

As I mentioned in #3728, I think that we should break this up into two independent tasks. One is to handle the dereferencing in extract_arg_depth, and the other is to handle issues for the terminal types reach by resolve (in read_arg), which would happen even without the use of the resolve feature. With pointer types, read_arg() has some issues when the pointer is NULL. Because of this, I don't want to test scenarios where we resolve to a NULL pointer and pass that NULL pointer to read_arg() (yet).

Also, for the example you have above:

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

From my testing, I see that read_arg() is passed a pointer to an int in this scenario. If we want to resolve the int value that the pointer points to, I think we need to call extract_arg_depth one more time in order to dereference, because read_arg() takes the integer "by value" instead of by pointer to an integer. So, I don't want to test this scenario until we make this work in the non-NULL case.

So, I think that I should add a test for when the first pointer is NULL, and when the second to last pointer is NULL, but I don't want to test the scenario where the last pointer is NULL, because that problem is not specific to resolve, so I want to solve that issue independently.

[netlify]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	e900a2c
🔍 Latest deploy log	https://app.netlify.com/projects/tetragon/deploys/691f8105a5cbb60008975550
😎 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.

[olsajiri]

so if we fail to resolve we still continue and store the 'unresolved data' which likely null, right? I think we can skip it

but how about we make this more generic and we start each argument data with '__u32 error' status and in case of failure we write just error != 0 as argument value.. in case of resolve failure we encode the depth into it

then on user space side the getArg would just read first uint32 from event reader to get the argument status
so all the extra retrieval of error depth from each probe type would not be needed, also probably the argument index will be clear, because you have the error for argument you are about to read, or skip reading if it's != 0

we could use this to store other errors that happen during argument encoding, which there are plenty

and into final event instead of the depth value, I'd add error string that in case of resolve failure would contain something like: "failed to resolve current->file->f_inode"

seems like this could save some cycles and prevent bogus arguments values, because IIUC if we bail from argument storing in the middle on kernel side the user side still tries to read the whole part