snapd mountinfo parser doesn't handle the special "\040(deleted)" suffix

Bug #1830024 reported by Zygmunt Krynicki
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
snapd
In Progress
Low
Arseniy Aharonov

Bug Description

The kernel can include a special in one of the strings returned in /proc/pid/mountinfo. This suffix is used when one of the objects referring to the mount has been deleted and the mount is no longer functional.

I realised this by reviewing a patch https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=86673b3a46

We don't currently expect to see such mount points during normal operation but then again, we should handle it explicitly.

Zygmunt Krynicki (zyga)
Changed in snapd:
assignee: nobody → Zygmunt Krynicki (zyga)
Changed in snapd:
assignee: Zygmunt Krynicki (zyga) → nobody
Revision history for this message
Arseniy Aharonov (arsenique) wrote (last edit ):

I've succeeded to reproduce the behaviour described by this ticket. One shall perform the following steps to reproduce it:
```
# unshare -m
# echo foo > foo; touch bar baz quux
# mount -B foo bar
# mount -B bar baz
# grep foo /proc/self/mountinfo
56 38 8:7 /tmp/foo /tmp/bar ...
57 38 8:7 /tmp/foo /tmp/baz ...

# rm foo
# grep foo /proc/self/mountinfo
56 38 8:7 /tmp/foo//deleted /tmp/bar ...
57 38 8:7 /tmp/foo//deleted /tmp/baz ...
```

Changed in snapd:
assignee: nobody → Arseniy Aharonov (arsenique)
Revision history for this message
Arseniy Aharonov (arsenique) wrote (last edit ):

The related functionality in c-sources of snapd actually duplicates what does the ''mnt_table_parse_fstab()'' API that can be easily populated with ''<libmount/libmount.h>''. This will fix all possible problem that might be related to the reported issue.

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

Yes but at the time we didn't choose to use libmount as that would introduce another dependency that would have to be up-to-date in the target distribution.

Revision history for this message
Arseniy Aharonov (arsenique) wrote (last edit ):

Could you explain in more details please? What are we trying to avoid by duplicating the code? Which dependency are we trying to avoid? Aren't we talking about the dependency that is already resolved?
This packages is mandatory for the Kernel to be functional. It is always present.

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

It's not about making sure the dependency is present. At the time this code was written, there were two concerns:

- the version of libmount across distributions was not uniform
- using libmount directly required additional apparmor profile permissions

This decision can be re-visited by the snapd team, I'm just providing the historic context.

Revision history for this message
Arseniy Aharonov (arsenique) wrote :

Well, till now three potential regions might be affected by this potential issue:
1. C-logic related to parsing of Mountinfo
2. bash scripts that are used to cleanup after a snap is removed
3. GoLang logic related to parsing of Mountinfo

Revision history for this message
Arseniy Aharonov (arsenique) wrote :

For the first phase, it was decided to leave the C-logic without changes due to complicated state of dependencies of this code. The code in question is very sensitive to security issues and demands very heavy auditing to prevent the risk of security breaches.

Bash scripts and Go-Lang require preliminary refactoring prior to fixes, to prevent maintainability issues (e.g. code duplication).

Changed in snapd:
status: Triaged → In Progress
Revision history for this message
Arseniy Aharonov (arsenique) wrote :

Hi,

After analyzing the `unitl-linux` of Ubuntu LTS, I've seen that the aforementioned patch with the fix is included.
However, the attempt to reproduce the failure shows that the `/(deleted)` prefix is attached to the root filed of the mount-info entry. Which made me think that what we need to focus on is to check how snapd itself is affected when parsing mountinfo records that reflect the status of broken link.

I've reproduced the failure once again on a VM and used snapd osutil package to load the "corrupted" moountinfo data, the same way as snapd does. Then, I printed the contents of the []*MountInfoEntry in JSON format (please see the attached reproduced_MountInfoEntry_structure.json). As one can see, snapd succeeded to process such file without crashing, and similar to reproduced results in command-line, the output is affected the way that the `/(deleted)` prefix is attached to the root filed. This means that we are not at risk of crashing due to this phenomena in the file.
Taking into account that this data doesn't crash snapd, it worth checking how this field is used by snapd, and currently there is no occurrence within the snapd code where this field is used rather then simply compared to '/'.

All this makes me to conclude that this doesn't pose any risks to at least go-part of snapd.

Kind regards,
Arseniy

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.