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

Bryam Vargas posted 1 patch 1 day, 5 hours ago
There is a newer version of this series
fs/isofs/rock.c | 11 +++++++++++
1 file changed, 11 insertions(+)
[PATCH] isofs: bound Rock Ridge symlink components to the SL record
Posted by Bryam Vargas 1 day, 5 hours ago
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 both walk sites.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
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 (verified: the
returned link contains neighbouring directory-record / slab contents,
not just the target).  On a tight continuation allocation the read
leaves the object and is a slab-out-of-bounds read that KASAN reports in
get_symlink_chunk().  With the patch the over-long component is rejected
(slp->len + 2 > slen) and readlink() returns only the valid prefix; 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 a two-byte header (flags, len) followed by
+		 * len bytes of text, i.e. slp->len + 2 bytes.  Stop if it does
+		 * not fit in the bytes left in the SL record, otherwise the
+		 * memcpy() of slp->text below reads past the record.
+		 */
+		if (slp->len + 2 > slen)
+			break;
 		switch (slp->flags & ~1) {
 		case 0:
 			if (slp->len > plimit - rpnt)
--
2.43.0
Re: [PATCH] isofs: bound Rock Ridge symlink components to the SL record
Posted by Michael Bommarito 1 day, 4 hours ago
On Sat, Jun 6, 2026 at 6:29 PM Bryam Vargas <hexlabsecurity@proton.me> wrote:
> 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;

Thanks for the CC.  You beat me to filing this one!

Your patch is better than the one I never got around to submitting,
but one note I'd mention is that I returned NULL here instead of
breaking so that readlink() would fail with -EIO downstream.  Maybe
I'm missing something elsewhere, but I think this design results in
silent truncation and a potentially confused caller who thinks the
symlink was successful.

Thanks,
Mike
Re: [PATCH] isofs: bound Rock Ridge symlink components to the SL record
Posted by Bryam Vargas 1 day, 3 hours ago
On Sat, 6 Jun 2026, Michael Bommarito <michael.bommarito@gmail.com> wrote:
> one note I'd mention is that I returned NULL here instead of breaking so
> that readlink() would fail with -EIO downstream. Maybe I'm missing
> something elsewhere, but I think this design results in silent truncation
> and a potentially confused caller who thinks the symlink was successful.

Agreed. In get_symlink_chunk() a break leaves rock_ridge_symlink_read_folio()
with rpnt != link, which it treats as success, so readlink() returns the
truncated target. v2 returns NULL there instead -- matching the existing
plimit-overflow checks in the same function -- so a malformed SL record now
fails with -EIO rather than silently truncating.

I left the break in parse_rock_ridge_inode_internal(): that site only bounds
the i_size walk, and the link is read back through get_symlink_chunk(), which
now errors on the same record, so the truncated i_size is never consumed. I'm
happy to make that site a hard error too if you'd rather both reject.

I checked the three behaviours on the exact loop, with one valid component
followed by an over-long one: unpatched -> 255-byte out-of-bounds read; break
-> readlink() returns the truncated "abc/"; return NULL -> -EIO; a well-formed
link is unaffected.

v2 below, with your suggestion.

Bryam