https://bugs.kde.org/show_bug.cgi?id=509566
--- Comment #2 from Mark Wielaard <[email protected]> --- Comment on attachment 185053 --> https://bugs.kde.org/attachment.cgi?id=185053 proposed patch +PRE(sys_mount_setattr) +{ + // int syscall(SYS_mount_setattr, int dirfd, const char *pathname, + // unsigned int flags, struct mount_attr *attr, size_t size); + *flags |= SfMayBlock; + Int arg_1 = (Int)ARG1; + const HChar *path = (const HChar*) ARG2; + PRINT("sys_mount_setattr ( %d, %#" FMT_REGWORD "x, %" FMT_REGWORD "u, %#" + FMT_REGWORD "x, %" FMT_REGWORD "u )", arg_1, ARG2, + ARG3, ARG4, ARG5); + if (!ML_(fd_allowed)(ARG1, "mount_setattr", tid, False)) + SET_STATUS_Failure( VKI_EBADF ); Below you do this same check differently. Probably shouldn't be here. + if(ARG2) + PRE_MEM_RASCIIZ( "mount_setattr(pathname)", ARG2); Is pathname allowed to be NULL? If not then the if(ARG2) shouldn't be used. + PRE_MEM_READ("mount(attr)", ARG5, sizeof(struct vki_mount_attr)); I think this should use the size argument instead: PRE_MEM_READ("mount(attr)", ARG5, ARG6); Where size normally is sizeof(*attr), but doesn't have to be (could be larger in newer kernels). + if (ML_(safe_to_deref) (path, 1)) { + if (path[0] != '/') { + if (arg_1 != VKI_AT_FDCWD && !ML_(fd_allowed)(arg_1, "mount_setattr", tid, False)) { + SET_STATUS_Failure( VKI_EBADF ); + return; + } + } This variant of the check looks correct. The return is not technically needed. Also note that Paul recently introduced a helper ML_(fd_at_check_allowed) that does the same. Probably more consistent to use that. + } +} PRE(sys_move_mount) { + Int arg_1 = (Int)ARG1; + const HChar *from_pathname = (const HChar*) ARG2; + Int arg_3 = (Int)ARG3; + const HChar *to_pathname = (const HChar*) ARG4; PRINT("sys_move_mount ( %ld, %#" FMT_REGWORD "x(%s), " "%ld, %#" FMT_REGWORD "x(%s), %ld", SARG1, ARG2, (HChar*)(Addr)ARG2, SARG3, ARG4, (HChar*)(Addr)ARG4, SARG5); - PRE_REG_READ5(long, "mount_move", + PRE_REG_READ5(long, "move_mount", int, from_dfd, const char *, from_pathname, int, to_dfd, const char*, to_pathname, int, flags); - PRE_MEM_RASCIIZ( "mount_move(from_pathname)", ARG2); + PRE_MEM_RASCIIZ( "move_mount(from_pathname)", ARG2); /* For absolute filenames, from_dfd is ignored. If from_dfd is AT_FDCWD, from_pathname is relative to cwd. When comparing from_dfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ - if (ML_(safe_to_deref)( (void*)(Addr)ARG2, 1 ) - && *(Char *)(Addr)ARG2 != '/' - && ((Int)ARG1) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG1, "mount_move", tid, False)) + if (ML_(safe_to_deref)(from_pathname, 1) + && (from_pathname[0] != '/') + && (arg_1 != VKI_AT_FDCWD) + && !ML_(fd_allowed)(arg_1, "move_mount", tid, False)) SET_STATUS_Failure( VKI_EBADF ); - PRE_MEM_RASCIIZ( "mount_move(from_pathname)", ARG4); + PRE_MEM_RASCIIZ( "move_mount(from_pathname)", ARG4); /* For absolute filenames, to_dfd is ignored. If to_dfd is AT_FDCWD, to_pathname is relative to cwd. When comparing to_dfd against AT_FDCWD, be sure only to compare the bottom 32 bits. */ - if (ML_(safe_to_deref)( (void*)(Addr)ARG4, 1 ) - && *(Char *)(Addr)ARG4 != '/' - && ((Int)ARG4) != ((Int)VKI_AT_FDCWD) - && !ML_(fd_allowed)(ARG3, "mount_move", tid, False)) + if (ML_(safe_to_deref)(to_pathname, 1) + && (to_pathname[0] != '/') + && (arg_3 != VKI_AT_FDCWD) + && !ML_(fd_allowed)(arg_3, "move_mount", tid, False)) SET_STATUS_Failure( VKI_EBADF ); } Thanks for spotting and fixing all the syscall name typos here (oops, my fault). While you are updating the code it probably would be nice to use the new ML_(fd_at_check_allowed) helper function here too. -- You are receiving this mail because: You are watching all bug changes.
