Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fillers for chmod syscalls #1472

Merged
merged 11 commits into from
Aug 13, 2019
Merged

fillers for chmod syscalls #1472

merged 11 commits into from
Aug 13, 2019

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Aug 8, 2019

I'm doing the fillers for chmod syscalls

  • fchmodat
  • fchmodat bpf
  • chmod
  • chmod bpf
  • fchmod
  • fchmod bpf

Todo checks before merge:

  • test on ia32
  • check the scap file

To be able to turn this:

sudo ./userspace/sysdig/sysdig  evt.type=fchmodat  -p" %evt.num %evt.outputtime %evt.cpu %proc.name (%thread.tid) %evt.dir %evt.type %evt.info"
 4155 16:43:11.118737167 1 chmod (6992) > fchmodat
 4156 16:43:11.118764695 1 chmod (6992) < fchmodat
To this:
sudo ./userspace/sysdig/sysdig  evt.type=fchmodat  -p" %evt.num %evt.outputtime %evt.cpu %proc.name (%thread.tid) %evt.dir %evt.type %evt.info"
 19492 16:37:31.888889616 0 chmod (6013) > fchmodat
 19493 16:37:31.888914324 0 chmod (6013) < fchmodat res=0 dirfd=-100(AT_FDCWD) filename=ciao(/home/ubuntu/sysdig/build/test) mode=1FD

To this:

sudo ./userspace/sysdig/sysdig  evt.type=fchmodat  -p" %evt.num %evt.outputtime %evt.cpu %proc.name (%thread.tid) %evt.dir %evt.type %evt.info"
 49942 10:40:07.150453566 3 a.out (17383) > chmod
 49943 10:40:07.150461060 3 a.out (17383) < chmod res=0 filename=/tmp/test mode=04000(S_ISUID)

Signed-off-by: Lorenzo Fontana [email protected]

@fntlnz fntlnz changed the title WIP: new(driver): filler for fchmodat WIP: fillers for chmod syscalls Aug 8, 2019
@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from a448008 to 3c36cd2 Compare August 9, 2019 13:00
@fntlnz fntlnz changed the title WIP: fillers for chmod syscalls fillers for chmod syscalls Aug 9, 2019
Copy link
Contributor

@gnosek gnosek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo some nits (and I'm not sure we need to bump the scap file version for this change).

Copy link
Contributor

@krisnova krisnova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests would be my only nag - but unsure how we handle tests yet - still learning the code base

@fntlnz
Copy link
Contributor Author

fntlnz commented Aug 12, 2019

Corrected the requirements:

We don't want mode to be reported in hex like mode=1FD nor as just the mode number like 04000 or 0777, we want the mode number and the breakdown of the modes names like 04000(S_ISUID) or mode=04111(S_IXOTH|S_IXGRP|S_IXUSR|S_ISUID) or mode=0777(S_IXOTH|S_IWOTH|S_IROTH|S_IXGRP|S_IWGRP|S_IRGRP|S_IXUSR|S_IWUSR|S_IRUSR)

@fntlnz
Copy link
Contributor Author

fntlnz commented Aug 12, 2019

Expected json output:

{
  "evt.cpu": 1,
  "evt.dir": ">",
  "evt.info": "",
  "evt.num": 72162,
  "evt.outputtime": 1565606887658010000,
  "evt.type": "chmod",
  "proc.name": "a.out",
  "thread.tid": 17912
}
{
  "evt.cpu": 1,
  "evt.dir": "<",
  "evt.info": "res=0 filename=/tmp/ciao mode=04000(S_ISUID) ",
  "evt.num": 72163,
  "evt.outputtime": 1565606887658017500,
  "evt.type": "chmod",
  "proc.name": "a.out",
  "thread.tid": 17912
}

@gnosek
Copy link
Contributor

gnosek commented Aug 12, 2019

For the format change, I'd do this in a separate PR, adding PF_MODE (PF_FILEMODE? let's bikeshed) and possibly updating other users of PF_OCT to use it (off the top of my head, creat, open* and mkdir* could benefit from it as well). Thoughts?

@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from 14d1ac5 to 96de564 Compare August 12, 2019 11:05
@fntlnz
Copy link
Contributor Author

fntlnz commented Aug 12, 2019

PT_MODES is a type of option
PF_OCT is formatting the value as an octect

The way the additional information are handled is not based on the format but on the option type, as it is for PT_FLAGS32 and others, so I just added a PT_MODES for the file modes.

I agree that we can use it in other places where mode is used but let's leave that for another PR.

At the same moment I don't want to add PT_MODES in another PR because I need it to finish this one and it wouldn't make sense to me to close this one without PT_MODES because it's the center of it.

@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from 96de564 to 72586a9 Compare August 12, 2019 12:52
@fntlnz fntlnz requested review from gnosek and leodido August 12, 2019 12:54
@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch 3 times, most recently from 72586a9 to ffb8e74 Compare August 12, 2019 16:00
@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from ffb8e74 to aea95f2 Compare August 12, 2019 17:47
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully you knew about https://github.com/draios/sysdig/wiki/Adding-an-event-to-sysdig. But never worry, it looks you followed all the right steps!

Couple of small-ish comments about the event flag and management of the string representation of the event.

@fntlnz fntlnz requested a review from mstemm August 13, 2019 10:25
@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from 56fda9c to 4a14a35 Compare August 13, 2019 10:27
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more iteration on the resizing and then I think it's ready!

…lue instead of doubling it

Signed-off-by: Lorenzo Fontana <[email protected]>
@fntlnz fntlnz force-pushed the feat/chmod-syscalls branch from 4a14a35 to 445bcbf Compare August 13, 2019 17:12
@mstemm mstemm self-requested a review August 13, 2019 17:43
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@fntlnz fntlnz merged commit e857be7 into dev Aug 13, 2019
@fntlnz fntlnz deleted the feat/chmod-syscalls branch August 13, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants