Skip to content

security: baseband_guard: fix two bugs in is_protected_blkdev causing…#78

Open
leegarchat wants to merge 1 commit intovc-teahouse:mainfrom
leegarchat:patch-1
Open

security: baseband_guard: fix two bugs in is_protected_blkdev causing…#78
leegarchat wants to merge 1 commit intovc-teahouse:mainfrom
leegarchat:patch-1

Conversation

@leegarchat
Copy link
Copy Markdown
Contributor

… panic and dead code

Two independent bugs in is_protected_blkdev(), called from the bb_inode_rename and bb_inode_setattr LSM hooks.

━━━━━━━━━━━━━━━━━━━━━━
Bug 1 — Inverted NULL guard (all protection logic is dead code)
━━━━━━━━━━━━━━━━━━━━━━

if (!IS_ERR_OR_NULL(dentry)) /* BUG: ! inverts the guard */
return 0;

IS_ERR_OR_NULL() returns true for NULL or err-pointer dentries. Negating it means the condition is true for every valid (non-NULL, non-error) dentry — i.e. every real rename or setattr call. The function returned 0 immediately, making every line below it unreachable dead code. Block device and symlink rename / setattr protection was silently disabled for all callers.

Fix:
if (IS_ERR_OR_NULL(dentry))
return 0;

━━━━━━━━━━━━━━━━━━━━━━
Bug 2 — page_get_link() called on fast symlinks → NULL deref → kernel panic
━━━━━━━━━━━━━━━━━━━━━━

const char *link = inode->i_op->get_link(dentry, inode, &done);

The code calls get_link() to resolve the symlink target and verify it does not point into /dev/block/by-name. On most filesystems this dispatches to page_get_link(), which reads the target from the inode's page-cache mapping.

"Fast symlinks" (tmpfs, ramfs, shmem — target length ≤ ~62 bytes on 64-bit) store the target string directly in inode->i_link. They do not set up a page-cache mapping; inode->i_mapping->a_ops is left NULL.

page_get_link() → __page_get_link() unconditionally dereferences inode->i_mapping->a_ops, causing:

Unable to handle kernel NULL pointer dereference at 0x0
__page_get_link+0xb4/0x144
page_get_link+0x18/0x48
bb_inode_rename+0x134/0x264
security_inode_rename+0x90/0x100
vfs_rename+0x154/0x514
Kernel panic — not syncing: Oops: Fatal exception

Trigger: rename of any short symlink on rootfs/tmpfs/ramfs, e.g.:
mv /etc /etc_link (AnyKernel3 setup_mountpoint on recovery rootfs)
mv /tmp /tmp_bak
rename("/system", "/system_orig", ...)

Symlinks in /dev/block/by-name always use long absolute targets (e.g. /dev/block/sde37) and are never fast symlinks, so they are unaffected. The protection goal — blocking renames that redirect by-name entries — is 100% preserved by this fix.

Fix: check inode->i_link before calling get_link(). Fast symlinks have inode->i_link set; the target string is already in memory and can be used directly. Slow symlinks have inode->i_link == NULL and fall through to the original get_link() path unchanged.

if (inode->i_link) {
symlink_target_link = inode->i_link; /* inline target, no page fault */
} else {
symlink_target_link = inode->i_op->get_link(dentry, inode, &done);
}

Note: Bug 1 is what prevented the panic in binaries compiled from this source after the inverted guard was introduced as an emergency workaround. Fixing Bug 1 (restoring correctness) re-exposes Bug 2 — both must be fixed together.

Reproducer (before fix, with correct Bug-1 guard):
insmod baseband_guard.ko
mv /etc /etc_link → instant kernel panic
(after fix) → rename allowed, no panic

Reported-by: LeeGarChat

… panic and dead code

Two independent bugs in is_protected_blkdev(), called from the
bb_inode_rename and bb_inode_setattr LSM hooks.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Bug 1 — Inverted NULL guard (all protection logic is dead code)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  if (!IS_ERR_OR_NULL(dentry))   /* BUG: ! inverts the guard */
      return 0;

IS_ERR_OR_NULL() returns true for NULL or err-pointer dentries.
Negating it means the condition is true for every valid (non-NULL,
non-error) dentry — i.e. every real rename or setattr call.  The
function returned 0 immediately, making every line below it unreachable
dead code.  Block device and symlink rename / setattr protection was
silently disabled for all callers.

Fix:
  if (IS_ERR_OR_NULL(dentry))
      return 0;

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Bug 2 — page_get_link() called on fast symlinks → NULL deref → kernel panic
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  const char *link = inode->i_op->get_link(dentry, inode, &done);

The code calls get_link() to resolve the symlink target and verify it
does not point into /dev/block/by-name.  On most filesystems this
dispatches to page_get_link(), which reads the target from the inode's
page-cache mapping.

"Fast symlinks" (tmpfs, ramfs, shmem — target length ≤ ~62 bytes on
64-bit) store the target string directly in inode->i_link.  They do not
set up a page-cache mapping; inode->i_mapping->a_ops is left NULL.

page_get_link() → __page_get_link() unconditionally dereferences
inode->i_mapping->a_ops, causing:

  Unable to handle kernel NULL pointer dereference at 0x0
    __page_get_link+0xb4/0x144
    page_get_link+0x18/0x48
    bb_inode_rename+0x134/0x264
    security_inode_rename+0x90/0x100
    vfs_rename+0x154/0x514
  Kernel panic — not syncing: Oops: Fatal exception

Trigger: rename of any short symlink on rootfs/tmpfs/ramfs, e.g.:
  mv /etc /etc_link          (AnyKernel3 setup_mountpoint on recovery rootfs)
  mv /tmp /tmp_bak
  rename("/system", "/system_orig", ...)

Symlinks in /dev/block/by-name always use long absolute targets
(e.g. /dev/block/sde37) and are never fast symlinks, so they are
unaffected.  The protection goal — blocking renames that redirect
by-name entries — is 100% preserved by this fix.

Fix: check inode->i_link before calling get_link().  Fast symlinks have
inode->i_link set; the target string is already in memory and can be
used directly.  Slow symlinks have inode->i_link == NULL and fall through
to the original get_link() path unchanged.

  if (inode->i_link) {
      symlink_target_link = inode->i_link;   /* inline target, no page fault */
  } else {
      symlink_target_link = inode->i_op->get_link(dentry, inode, &done);
  }

Note: Bug 1 is what prevented the panic in binaries compiled from this
source after the inverted guard was introduced as an emergency workaround.
Fixing Bug 1 (restoring correctness) re-exposes Bug 2 — both must be
fixed together.

Reproducer (before fix, with correct Bug-1 guard):
  insmod baseband_guard.ko
  mv /etc /etc_link          → instant kernel panic
  (after fix)                → rename allowed, no panic

Reported-by: LeeGarChat
@AlexLiuDev233
Copy link
Copy Markdown
Collaborator

... did it actually exist?
it already fixed 2days ago
and your pr even can't pass compile...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants