On Sat, Aug 24, 2024 at 3:14 AM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Sat, Aug 24, 2024 at 5:13 AM Luis Chamberlain <mcgrof@xxxxxxxxxx> 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.)
