GRMLBASE: Fixes and improments for forensic mode #419
Conversation
…arent
When parent_device is empty, `grep -qw "${parent_device}"` will always
return true regardless of input. So when READONLY_IGNORE is set and no
parent device is found for a device, that device is never marked readonly.
However, newly seen devices should be marked as readonly unless otherwise
specified (eg. in READONLY_IGNORE). Ensure that devices with no parent are
marked as readonly.
Signed-off-by: Glenn Washburn <[email protected]>
…READONLY_IGNORE The script never checked for the block device in READONLY_IGNORE, but it appears that this was intended. So a new block device would be set to readonly, even if it was specified in READONLY_IGNORE. Signed-off-by: Glenn Washburn <[email protected]>
There is no need to depend on a separate binary when the shell's builtin `echo` function will suffice. Signed-off-by: Glenn Washburn <[email protected]>
…device-mapper device Parent devices of device-mapper devices use a directory named "holders" in their sysfs block device directory that lists all device-mapper devices which use that device. This is unlike partition devices of block devices which have directory directly in the sysfs block device directory. So if the current device name is not found directly in the sysfs block device directory, then check the "holders" directory. Signed-off-by: Glenn Washburn <[email protected]>
|
Hi, thanks for the PR! The build-and-push failure is on me (and I'll try to fix that), however the reported shellcheck issues are, I think, real. Please check them :-) |
… parents Some block devices can have multiple parents, such as LVM raid volumes. Signed-off-by: Glenn Washburn <[email protected]>
Signed-off-by: Glenn Washburn <[email protected]>
|
Thanks for working on this, I like what I see but need some more time to properly review/test it. |
Checking in, in case this dropped off the radar. |
Thanks, definitely didn't forget it, just didn't have time yet, will try to take care in the next days! Thanks |
|
Ping. Still not forgotten? |
Sorry, definitely not forgotten, will try to take care this week! |
mika
left a comment
mika
left a comment
There was a problem hiding this comment.
I like your changes, they all make sense for me, I have just some minor questions, suggestions and nitpicks.
I'm curious though, in what scenario did you stumble upon this, would you have any STR (Steps To Reproduce) for us, to verify the existing vs new behavior from your PoV?
Thanks! 🙇
| parent_devices=$(findmnt -n -o source --target "$backingfile") | ||
|
|
||
| # Set loop device to read-write, will be set to readonly later if need be | ||
| blockdev --setrw "${BLOCK_DEVICE}" |
There was a problem hiding this comment.
Is there any reason why we should set this to rw by default instead of leaving it as-is?
E.g. the squashfs file that's usually behind /dev/sr0 is set to read-only already, no reason to set it temporarily to rw, nor?
| blockdev --setrw "${BLOCK_DEVICE}" | ||
|
|
||
| # Check if either the backing file's block device or filesystem are read-only | ||
| if is_ro "$parent_devices" || findmnt -n -o OPTIONS --target "$backingfile" | grep -E '^ro,|,ro,|,ro$'; then |
There was a problem hiding this comment.
The grep returns its match on stdout, use grep -qE ... instead?
| else | ||
| logger -t forensic-mark-readonly "not setting '${BLOCK_DEVICE}' to read-only as present in ignore list" | ||
| if [ -n "${parent_devices}" ] ; then | ||
| patternsfile=$(mktemp /dev/shm/forensic-mark-readonly.patterns.XXXXXX) |
There was a problem hiding this comment.
Is there any reason why we'd want to hard-code the directory /dev/shm/ here?
Otherwise I'd prefer mktemp -t forensic-mark-readonly.patterns.XXXXXX instead, so mktemp(1) can decide whether $TMPDIR is set, or use /tmp elsewhere.
| patternsfile= | ||
| cleanup() { | ||
| if [ -f "$patternsfile" ] ; then | ||
| rm "$patternsfile" |
There was a problem hiding this comment.
let's maybe use rm -f "${patternsfile}" to not (report) fail(ure) if removing should fail?
|
|
||
| patternsfile= | ||
| cleanup() { | ||
| if [ -f "$patternsfile" ] ; then |
There was a problem hiding this comment.
nit: use "${patternsfile}" instead of "$patternsfile" to keep coding style style consistent, same for e.g "$backingfile" etc further below
| logger -t forensic-mark-readonly "not setting '${BLOCK_DEVICE}' (parent device: '${parent_device}') to read-only as present in ignore list" | ||
| else | ||
| logger -t forensic-mark-readonly "not setting '${BLOCK_DEVICE}' to read-only as present in ignore list" | ||
| if [ -n "${parent_devices}" ] ; then |
There was a problem hiding this comment.
nit: maybe use "${parent_devices:-}" also here, as it used to be?
3 participants
Some changes in this patch series: