[PATCH v2] isofs: bound Rock Ridge symlink components to the SL record

[Bryam Vargas <hexlabsecurity@proton.me>]

get_symlink_chunk() and the SL handling in
parse_rock_ridge_inode_internal() walk the variable-length components of
a Rock Ridge "SL" (symbolic link) record.  Each component is a two-byte
header (flags, len) followed by len bytes of text, so it occupies
slp->len + 2 bytes.  Both loops read slp->len and advance to the next
component, and get_symlink_chunk() additionally does
memcpy(rpnt, slp->text, slp->len), but neither checks that the component
lies within the SL record before dereferencing it.

A crafted SL record whose component declares a len that runs past the
record (rr->len) therefore triggers an out-of-bounds read of up to 255
bytes.  When the record sits at the tail of its backing buffer - for
example a small kmalloc()ed continuation block reached through a CE
record - the read crosses the allocation; get_symlink_chunk() then
copies the out-of-bounds bytes into the symlink body returned to user
space by readlink(), disclosing adjacent kernel memory.

ISO 9660 images are routinely mounted from untrusted removable media -
desktop environments auto-mount them (e.g. via udisks2) without
CAP_SYS_ADMIN - so the record contents are attacker-controlled.

Reject any component that does not fit in the remaining record bytes
before using it.  In get_symlink_chunk() return NULL, like the existing
output-buffer (plimit) checks, so a malformed record makes readlink()
fail with -EIO rather than silently returning a truncated target; in
parse_rock_ridge_inode_internal() stop the inode-size walk.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Suggested-by: Michael Bommarito <michael.bommarito@gmail.com>
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: in get_symlink_chunk() return NULL (-> readlink() -EIO) instead of
    break on an over-long component, per Michael Bommarito's review.
    break would let rock_ridge_symlink_read_folio() treat the truncated
    rpnt as success and hand a silently-truncated symlink to userspace;
    NULL matches the function's existing plimit-overflow handling and
    fails closed.  The size walk in parse_rock_ridge_inode_internal()
    keeps break (it only bounds i_size; the read path now errors).

Reproducer (crafted ISO-9660 image with Rock Ridge):

  # a symlink whose SL component length byte is enlarged so the
  # component overruns its SL record
  ln -s "$(python3 -c 'print("A"*250)')" /tmp/iso/l
  genisoimage -R -o rr.iso /tmp/iso
  # repoint the symlink's CE record to a tight continuation block
  # (cont_size = 7) holding one SL record:
  #   53 4c 07 01 00 00 ff   "SL", len 7, ver 1, comp flags 0, comp len 0xff
  # so rock_continue() does kmalloc(7) and the component text begins at
  # the end of that allocation.
  mount -o loop,ro rr.iso /mnt
  readlink /mnt/l

Without the patch, get_symlink_chunk() memcpy()s slp->len (0xff) bytes
starting one byte into the 7-byte allocation, so readlink() returns the
symlink target followed by adjacent in-kernel bytes.  With the patch the
over-long component is rejected (slp->len + 2 > slen) and readlink()
fails with -EIO; a well-formed Rock Ridge image is unaffected.

 fs/isofs/rock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index 1232fab59a4e..0fe781381e66 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -466,6 +466,9 @@ parse_rock_ridge_inode_internal(struct iso_directory_record *de,
 				inode->i_size = symlink_len;
 				while (slen > 1) {
 					rootflag = 0;
+					/* keep the component within the SL record */
+					if (slp->len + 2 > slen)
+						break;
 					switch (slp->flags & ~1) {
 					case 0:
 						inode->i_size +=
@@ -621,6 +624,14 @@ static char *get_symlink_chunk(char *rpnt, struct rock_ridge *rr, char *plimit)
 	slp = &rr->u.SL.link;
 	while (slen > 1) {
 		rootflag = 0;
+		/*
+		 * A component is slp->len + 2 bytes (a two-byte header plus
+		 * len bytes of text).  If it does not fit in the bytes left in
+		 * the SL record the record is malformed: fail like the plimit
+		 * checks below so readlink() returns -EIO, not a truncated path.
+		 */
+		if (slp->len + 2 > slen)
+			return NULL;
 		switch (slp->flags & ~1) {
 		case 0:
 			if (slp->len > plimit - rpnt)
--
2.43.0