On Sat, Aug 24, 2024 at 3:14 AM Linus Torvalds torvalds@linux-foundation.org wrote:
On Sat, Aug 24, 2024 at 5:13 AM Luis Chamberlain mcgrof@kernel.org wrote:
I'm all for this, however a strong rejection outright for the first kernel release is bound to end up with some angry user with some oddball driver that had this for whatever stupid reason.
I can't actually see a reason why a firmware file would have a ".." component in it, so I think the immediate rejection is fine - particularly since it has a warning printout, so you see what happened and why.
I do wonder if we should just have a LOOKUP_NO_DOTDOT flag, and just use that.
[ Christian - the issue is the firmware loading path not wanting to have ".." in the pathname so that you can't load outside the normal firmware tree. We could also use LOOKUP_BENEATH, except kernel_read_file_from_path_initns() just takes one long path rather than "here's the base, and here's the path". ]
One other difference between the semantics we need here and LOOKUP_BENEATH is that we need to allow *symlinks* that contain ".." components or absolute paths; just the original path string must not contain them. If root decides to put symlinks to other places on the disk into /lib/firmware, I think that's reasonable, and it's root's decision to make, and we shouldn't break that. (And as an example, on my Debian machine, I see that /lib/firmware/regulatory.db is a symlink to /etc/alternatives/regulatory.db, which in turn is a symlink to /lib/firmware/regulatory.db-debian. I also see a bunch of symlinks in subdirectories of /lib/firmware with ".." in the link destinations, though those don't escape from /lib/firmware.)
So if we do this with a lookup flag, it'd have to be something that only takes effect when nd->depth is 0, or something vaguely along those lines? IDK how exactly that part of the path walking code works.
There might be other people who want LOOKUP_NO_DOTDOT for similar reasons. In fact, some people might want an even stronger "normalized path" validation, where empty components or just "." is invalid, just because that makes pathnames ambiguous.
(For what it's worth, I think I have seen many copies of this kind of string-based checking for ".." components in various pieces of userspace code. I don't think I've seen many places in the kernel that would benefit from that.)