proc: move procfs to what kernfs does

Ok, as mentioned off-list I have an RFC version of getting rid of page fault induced stalls in procfs when removing entry which plagues networking devices among others.

There was a hackish very narrow AI solution to a part of this bug class that was floated off-list. It used a second buffering mechanism on top of seqfiles which was really nasty. Plus, it didn't fix the whole bug class, it only eliminated a small portion. If we want to get rid of the whole page fault issue we need a bigger hammer. So I tried to work through that over the last couple of weeks. The series in a shape where it's not totally insane to have someone else look at it. So here it is.

I understand it's big but I thought it's probably nicer to look at the end result than just making a promise of what it might end up looking like.

It's not super polished yet and I haven't gotten into sorting out all the warts as there were some really gnarly bugs in there like the whole readdir mess that repeated entries or terminated early. I think we could go a lot further there if we wanted to but I think that blows this out of proportion and at some point it becomes difficult to grasp the scope. So I'd rather delay that until I have a sense of how acceptable this all is.

remove_proc_entry() waits until every call into the entry has returned, and most of those calls copy to or from userspace with the entry pinned.

A reader or writer can make a page fault in that copy take as long as it wants, with userfaultfd or by punching holes into a memfd over and over. And the callers of remove_proc_entry() like to hold locks while they wait. addrconf holds RTNL when it removes /proc/net/dev_snmp6/<dev> and xt_hashlimit holds a mutex that all network namespaces share.

Every network device with IPv6 gets a /proc/net/dev_snmp6/<dev> that everyone can read, and addrconf_ifdown() removes it with RTNL held. So any local user can keep RTNL for as long as their read stays stalled. Until then no device can be registered, removed or renamed and no network namespace can be created or torn down.

Without privileges a FUSE mount in a user namespace whose daemon never answers is enough to stall the copy. Killing the reader ends it, but until then whoever removes the file sits in D state.

xt_hashlimit removes the proc file of a table with hashlimit_mutex held. Every rule insert in every network namespace takes that mutex and so does network namespace cleanup. cleanup_net() runs on a single workqueue with pernet_ops_rwsem held, so a stalled reader also stops every later network namespace teardown and every module load that registers pernet operations. xt_recent does the same with recent_mutex.

And it's not specific to networking. remove_proc_entry(), proc_remove() and remove_proc_subtree() have over 200 callers and plenty of them hold locks. A stalled reader of /proc/fs/ext4/<dev>/options holds up the unmount of that filesystem with s_umount held, and every sync(2) with it. The same goes for PCI hot-removal, for freeing IRQ descriptors, which CPU bring-up waits on, and for unplugging a USB sound card. syzbot has hit removal under locks before (cf. [1]), and the xt_recent deadlock fixed in (1) below is another one.

Stalling the kernel in a copy to or from userspace is a well-known exploitation primitive. It's why unprivileged userfaultfd can't do it by default anymore since v5.11 (d0d4730ac2e4), and FUSE has been used the same way.

kernfs doesn't have that problem. The code of a kernfs file only ever runs with the node pinned and never touches userspace memory, kernfs copies in and out itself. This moves procfs over to the same scheme.

sysfs has only held its active reference around ->show() and ->store() since v2.6.23, the same release that made remove_proc_entry() wait for the entry's callers. A copy to userspace couldn't be stalled for long back then. userfaultfd only came in v4.3 and FUSE in user namespaces in v4.18. configfs moved its copies out of its active reference in v5.3 and DRM copies ioctl arguments in and out around the driver's handler the same way. debugfs still holds its reference across the copies, ran into a deadlock with wifi drivers because of it and grew an API to cancel operations instead (cf. [2]).

That touches just about every ->proc_read(), ->proc_write() and ->proc_ioctl() in the tree, so I've split it up into seven series. They go out as replies to this mail:

(1) proc: fix a few old bugs in /proc handlers (38 patches)

Bugs in the handlers and in procfs itself that are a lot older than the conversion and that reviewing it turned up. Most of them are for stable. They come first so they can go there on their own and the conversion doesn't have to bother with that crap.

(2) proc: let procfs own the data path (16 patches)

The procfs side. procfs owns the seq_file of an entry, copies in writes, copies out reads and copies ioctl arguments in and out, and only calls into an entry with it pinned. proc_create_seq() and friends and the plain seq_file users are switched over here too.

(3) proc: convert the arch and platform entries (22 patches)
(4) proc: convert the driver entries (23 patches)
(5) proc: convert the fs, kernel and mm entries (27 patches)
(6) proc: convert the net entries and the kfifo samples (11 patches)

(7) proc: drop the __user methods (5 patches)

    Drops the old methods for every entry that can be removed and gives
    the new ones the names kernfs uses, ->proc_read(), ->proc_write()
    and ->proc_ioctl(). This needs all four conversion series.

The series should all indicate cleanly what dependencies they have so b4 should be able to pull it for review. But I could be wrong.

With all of it on a KASAN and lockdep kernel the removal selftests of every part pass, the proc, pktgen and allocinfo selftests behave like without it and the bugs from (1) that reproduce in a VM are gone. Every patch builds on its own on x86_64 and i386, and with W=1 on the architectures it touches.

[1]: https://lore.kernel.org/r/[email protected]
[2]: https://lore.kernel.org/r/[email protected]

Christian Brauner <[email protected]>

Moving procfs to what kernfs does touches just about every ->proc_read(), ->proc_write() and ->proc_ioctl() in the tree. Reviewing that turned up a bunch of bugs that have nothing to do with the conversion and are a lot older. They're fixed here first so they can go to stable on their own and the conversion doesn't have to carry them.

Two of them matter for the conversion itself:

- proc_bus_pci_write() truncates the file position to an int. A pwrite() at an offset like 0xfffffc00 makes it negative and the loop then writes a lot more than it was handed into config space at negative offsets. Once procfs hands it a kernel copy of the write that's a heap out-of-bounds read, so this has to be fixed before the PCI conversion:

    BUG: KASAN: slab-out-of-bounds in proc_bus_pci_write+0x528/0x5c0
    Read of size 4 at addr ff11000112103b20 by task pci-check/241
    Call Trace:
     proc_bus_pci_write+0x528/0x5c0
     proc_reg_write+0x2d7/0x730
     vfs_write+0x1cd/0xfa0
     __x64_sys_pwrite64+0x191/0x250

isapnp has the same bug on the read side.

- xt_recent removes the proc entries of its tables with a spinlock held when a network namespace goes away. remove_proc_entry() waits for the readers and a reader takes that very lock. That's a deadlock.

/proc/sys readdir resumes a listing at a counted position. The entries are sorted by name, so a device that comes or goes in front of that position shifts them and the next getdents64() repeats an entry or skips one. Even ls needs a second call to find the end of a directory. It resumes after the name of the last entry it returned now, the way kernfs resumes after its last node, and a selftest lists a directory in two pieces around a device coming and going. That one is first, it goes to stable on its own.

/proc/sys also has the bug the conversion fixes for the rest of /proc. proc_sys_call_handler() copies what is read and written and proc_sys_readdir() copies the names of the entries to and from userspace while the sysctl tables are in use. Unregistering a table waits until nobody uses it anymore. So a read(), write() or getdents64() whose buffer faults for as long as userspace wants keeps unregister_sysctl_table() waiting. addrconf unregisters the ipv6 sysctls of a device under RTNL, so that stalls every RTNL user. The handler copies in before it grabs the table and out after letting go of it. readdir doesn't take the tables into use at all anymore. It pins the memory of the entry it is at, which unregistering doesn't wait for, finds the next one under sysctl_lock and emits with nothing held, the way kernfs_fop_readdir() and proc_readdir_de() work. That's the same one trip through sysctl_lock per entry as before. A selftest covers all three.

/proc symlinks have it as well. proc_get_link() pins the entry and vfs_readlink() copies the link to userspace before the delayed call unpins it. ALSA removes /proc/asound/<id> with snd_card_mutex and info_mutex held when a card changes its id, so a faulting readlink() holds those up. The pin keeps nothing alive, the inode holds the entry, so it goes. A selftest covers that too.

Chris' review of the conversion turned up more old bugs, in procfs itself and in its users. Those are here as well:

- __xlate_proc_name() walks through whatever it finds for the components of a name, regular files included, and pktgen hands it the names that the writer of "add_device" picks. So an unprivileged user in namespaces of its own can hang an entry below another file and get it freed behind pktgen's back. procfs doesn't walk through anything but directories anymore and pktgen refuses names with a '/'.

- SUNRPC moved the waitqueue that poll() and epoll use for its channel files into the cache_detail of a network namespace. That's freed when the namespace goes away, but procfs leaves the waiters of a removed file queued. That's a use-after-free an unprivileged user can trigger too. It goes back to a single static waitqueue. procfs should really own the waitqueues of its files like kernfs does. I'll do that separately, it's nothing for stable.

- proc_register() WARNs when a name is already registered, and pktgen lets userspace pick names that collide with its own files. It reports the duplicate with pr_warn() and a stack dump now, like sysfs does.

- proc_readdir_de() resumes at a counted position just like /proc/sys and on top of that ends a listing early when the entry it is emitting goes away. It resumes after the last name now too, and a selftest covers both.

- proc_remove() removes by name, so removing an entry that is already gone can take out a different one. It removes the entry it's given now. And procfs warns about entries that a dying network namespace leaves in /proc/net instead of silently freeing their parent.

- On NOMMU every /proc file can be mapped privately, which reads it with nommu_region_sem held, and reading /proc/maps takes that lock too. Any user can hang the machine with that. procfs files can't be mapped on NOMMU anymore.

- esas2r takes the lengths of its HBA, SMP and firmware ioctls from userspace without checking them against the buffer they're copied into, through a file everyone can open.

- vmcore_cleanup() can run twice at once. powerpc's copy_oldmem_page() doesn't check ioremap_cache() and only recovers from a machine check for read(), not for splice() or vmcore's own reads. The pseries ofdt parser can read past its input and lparcfg reports a failed hcall as a huge write.

The rest is small: dasd looks in front of its buffer on an empty write, toshiba_acpi's input checks can't fail, simdisk reads the file name without the lock that protects it, rtas_flash can read past its message buffer, rtasd hands out whatever vmalloc() left in its log buffer, ALSA's info layer doesn't clamp writes past the end of a data entry, BusLogic, ips and libipw return 0 from writes so stdio writers spin forever, the scsi host files and um's exitcode parse what is written without NUL-terminating it, and ntfs3 warns when a huge label is written.

The PCI, xt_recent, sysctl, readdir, readlink, pktgen, SUNRPC, duplicate name and /proc readdir bugs reproduce on a KASAN and lockdep kernel in a VM and are gone with the series. The rest is build tested only.

The procfs conversion is on top of this, starting with "proc: let procfs own the data path".

---
Christian Brauner (38):
PCI/proc: don't truncate the position of config space accesses
PNP: isapnp: don't read past the end of the buffer on a large pread()
netfilter: xt_recent: don't remove the proc entries under a spinlock s390/dasd: don't look in front of the buffer on an empty write platform/x86: toshiba_acpi: reject malformed brightness and fan writes
xtensa: simdisk: don't read the file name without the lock powerpc/rtas_flash: don't read past the validate message powerpc/rtasd: zero the error log buffer
ALSA: info: don't write past the end of a data entry
scsi: BusLogic: don't return 0 from writes to the proc file
scsi: ips: don't return 0 from writes to the proc file
wifi: libipw: don't return 0 from writes to debug_level
scsi: core: NUL-terminate what is written to the host proc files
um: NUL-terminate what is written to /proc/exitcode
ntfs3: don't warn when a huge label is written
proc: don't walk through a non-directory when resolving a name
net: pktgen: reject device names with a '/'
proc: don't WARN about a name that is already registered
proc: resume readdir after the last name it returned selftests/proc: check a /proc listing while entries come and go
proc: let proc_remove() remove the entry it was given
proc: warn about entries left in /proc/net of a dying namespace
proc: don't let NOMMU map procfs files
proc/vmcore: don't let vmcore_cleanup() run twice at once powerpc/crash_dump: check ioremap_cache() in copy_oldmem_page() powerpc/crash_dump: recover from a machine check in copy_oldmem_page()
sunrpc: make queue_wait a global wait queue again
scsi: esas2r: bound the lengths of the HBA, SMP and firmware ioctls powerpc/pseries: bound the lengths in the ofdt property parser powerpc/pseries/lparcfg: return h_get_ppp()'s status as a long
proc: sysctl: resume readdir after the last name it returned selftests/proc: check a sysctl listing while entries come and go
proc: sysctl: copy to and from userspace without holding the table
proc: sysctl: look at the table under sysctl_lock when creating an inode
proc: sysctl: don't hold the tables while readdir copies to userspace selftests/proc: check that faulting sysctl accesses don't hold up removal
proc: don't pin a symlink's entry while readlink() copies it out selftests/proc: check that a faulting readlink() doesn't hold up removal

 arch/powerpc/kernel/crash_dump.c                   |   6 +-
 arch/powerpc/kernel/rtas_flash.c                   |   2 +-
 arch/powerpc/kernel/rtasd.c                        |   2 +-
 arch/powerpc/platforms/pseries/lparcfg.c           |   2 +-
 arch/powerpc/platforms/pseries/reconfig.c          |  18 +-
 arch/um/kernel/exitcode.c                          |   3 +-
 arch/xtensa/platforms/iss/simdisk.c                |  25 +-
 drivers/net/wireless/intel/ipw2x00/libipw_module.c |   2 +-
 drivers/pci/proc.c                                 |  15 +-
 drivers/platform/x86/toshiba_acpi.c                |   8 +-
 drivers/pnp/isapnp/proc.c                          |   6 +-
 drivers/s390/block/dasd.c                          |   2 +-
 drivers/scsi/BusLogic.c                            |   2 +-
 drivers/scsi/esas2r/esas2r.h                       |   2 +-
 drivers/scsi/esas2r/esas2r_ioctl.c                 |  41 ++-
 drivers/scsi/esas2r/esas2r_main.c                  |   3 +-
 drivers/scsi/ips.c                                 |   2 +-
 drivers/scsi/scsi_proc.c                           |   1 +
 fs/ntfs3/super.c                                   |  11 +-
 fs/proc/generic.c                                  | 174 ++++++---
 fs/proc/inode.c                                    |  38 +-
 fs/proc/internal.h                                 |   1 +
 fs/proc/proc_net.c                                 |   7 +
 fs/proc/proc_sysctl.c                              | 389 ++++++++++++++-------
 fs/proc/root.c                                     |   1 +
 fs/proc/vmcore.c                                   |   5 +
 include/linux/sunrpc/cache.h                       |   1 -
 net/core/pktgen.c                                  |   4 +
 net/netfilter/xt_recent.c                          |   4 +-
 net/sunrpc/cache.c                                 |  12 +-
 sound/core/info.c                                  |   2 +
 tools/testing/selftests/proc/.gitignore            |   5 +
 tools/testing/selftests/proc/Makefile              |   5 +
 tools/testing/selftests/proc/config                |   6 +
 tools/testing/selftests/proc/proc-readdir-churn.c  | 236 +++++++++++++
 .../selftests/proc/proc-remove-slow-readlink.c     | 191 ++++++++++
 .../selftests/proc/proc-remove-slow-sysctl.c       | 213 +++++++++++
 .../selftests/proc/proc-sysctl-readdir-churn.c     | 147 ++++++++
 38 files changed, 1342 insertions(+), 252 deletions(-)

---
base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
change-id: 20261101-work-procfs-bugfixes-054d00028491

proc_bus_pci_write() truncates the file position to an int. A pwrite() at an offset like 0xfffff000 turns it negative, the check against the size of the config space passes and the count it clamps to ends up larger than what was written. The loop then reads past the end of the caller's buffer and writes all of it into config space at negative offsets, which pci_user_write_config_*() pass on to the config accessors unchecked. The write also returns more than it was asked to write.

An offset above 4G that truncates to a small positive value writes to the start of config space instead. proc_bus_pci_read() keeps the position in an unsigned int, so a read at such an offset returns the start of config space instead of hitting EOF.

Check the position against the size of the config space before narrowing it.

---
 drivers/pci/proc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

index 3d7a9ed5fc22..e0dc70123f82 100644
@@ -32,8 +32,7 @@ static ssize_t proc_bus_pci_read(struct file *file, char __user *buf,
 				 size_t nbytes, loff_t *ppos)
 {
 	struct pci_dev *dev = pde_data(file_inode(file));
-	unsigned int pos = *ppos;
-	unsigned int cnt, size;
+	unsigned int pos, cnt, size;
 
 	/*
 	 * Normal users can read only the standardized portion of the

@@ -51,8 +50,9 @@ static ssize_t proc_bus_pci_read(struct file *file, char __user *buf,
 	if (!nbytes)
 		return 0;
 
-	if (pos >= size)
+	if (*ppos >= size)
 		return 0;
+	pos = *ppos;
 	if (nbytes >= size)
 		nbytes = size;
 	if (pos + nbytes > size)

@@ -119,9 +119,8 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
 {
 	struct inode *ino = file_inode(file);
 	struct pci_dev *dev = pde_data(ino);
-	int pos = *ppos;
 	int size = dev->cfg_size;
-	int cnt, ret;
+	int pos, cnt, ret;
 
 	ret = security_locked_down(LOCKDOWN_PCI_ACCESS);
 	if (ret)

@@ -130,14 +129,16 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
 	if (!nbytes)
 		return 0;
 
+	if (*ppos >= size)
+		return 0;
+	pos = *ppos;
+
 	if (resource_is_exclusive(&dev->driver_exclusive_resource, pos, nbytes)) {
 		pci_warn_once(dev, "%s: Unexpected write to kernel-exclusive config offset %x",
 			      current->comm, pos);
 		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 	}
 
-	if (pos >= size)
-		return 0;
 	if (nbytes >= size)
 		nbytes = size;
 	if (pos + nbytes > size)

2.53.0

I think this would actually be better with rw_verify_area() just checking a per-file maxbytes.

We already have that logic for lseek and friends - using that fairly hacky combination of s_maxbytes and !unsigned_offsets().

We should combine s_maxbytes and unsigned_offsets() at file open time into f_maxbytes and check that not just at lseek but in rw_verify_area.

And then things like proc_bus_pci could set it's size limit in one single place.

Hmm?

Linus

Yeah, I like that.

Hm, maybe we should keep this and the isapnp thing small and do that change on top.

This also might mean a change in behavior. Because a read past the end returns 0 for proc_bus_pci and for a regular file past s_maxbytes. So that would potentially need some more changes.

isapnp_proc_bus_read() truncates the position to an int. A pread() at an offset like 0xfffffc00 turns it negative, the check against the size of the configuration space passes, and the loop then puts everything from that position up to byte 256 into the reader's buffer, no matter how few bytes were asked for.

Keep the position as a loff_t and clamp the count against what is left of the configuration space.

---
 drivers/pnp/isapnp/proc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index c1f6e6bd0eb2..8197c7f29743 100644
@@ -23,14 +23,12 @@ static ssize_t isapnp_proc_bus_read(struct file *file, char __user * buf,
 				    size_t nbytes, loff_t * ppos)
 {
 	struct pnp_dev *dev = pde_data(file_inode(file));
-	int pos = *ppos;
+	loff_t pos = *ppos;
 	int cnt, size = 256;
 
 	if (pos >= size)
 		return 0;
-	if (nbytes >= size)
-		nbytes = size;
-	if (pos + nbytes > size)
+	if (nbytes > size - pos)
 		nbytes = size - pos;
 	cnt = nbytes;
 

2.53.0

recent_proc_net_exit() removes the proc entries of all tables with recent_lock held. remove_proc_entry() waits until nobody calls into the entry anymore, and reading one of them takes recent_lock in recent_seq_start() while procfs has the entry pinned. So when a reader gets there while the network namespace goes away, the exit path sleeps with a spinlock held and bh disabled, waiting for a reader that spins on that very lock:

  BUG: sleeping function called from invalid context at kernel/sched/completion.c:116
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 12, name: kworker/u16:0
  locks held by kworker/u16:0/12: 4, last CPU#2:
   #2: (pernet_ops_rwsem){++++}-{4:4}, at: cleanup_net+0xbf/0x860
   #3: (recent_lock){+...}-{3:3}, at: recent_net_exit+0x3d/0x170 [xt_recent]
  Workqueue: netns cleanup_net
  Call Trace:
   wait_for_completion+0x8e/0x400
   proc_entry_rundown+0x1ab/0x1f0
   remove_proc_entry+0x1ab/0x530
   recent_net_exit+0x9f/0x170 [xt_recent]
   ops_undo_list+0x271/0x8d0
   cleanup_net+0x3f3/0x860
   process_one_work+0x946/0x1820
  BUG: scheduling while atomic: kworker/u16:0/12/0x00000202

  watchdog: BUG: soft lockup - CPU#0 stuck for 26s! [rloop:252]
  Call Trace:
   _raw_spin_lock_bh+0x7a/0x80
   recent_seq_start+0x86/0x1e0 [xt_recent]
   seq_read_iter+0x26d/0x1230
   seq_read+0x2a9/0x560
   proc_reg_read+0x1c2/0x3c0
   vfs_read+0x176/0xb40
   __x64_sys_pread64+0x191/0x250

Tables are added and removed with recent_mutex held and that's also what recent_mt_destroy() looks at ->xt_recent under. Nothing that runs with a proc entry pinned takes it. So take recent_mutex instead.

---
 net/netfilter/xt_recent.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 55f1f2d89952..11acb9a50d4b 100644
@@ -652,12 +652,12 @@ static void __net_exit recent_proc_net_exit(struct net *net)
 	 * that the parent xt_recent proc entry is empty before trying to
 	 * remove it.
 	 */
-	spin_lock_bh(&recent_lock);
+	mutex_lock(&recent_mutex);
 	list_for_each_entry(t, &recent_net->tables, list)
 	        remove_proc_entry(t->name, recent_net->xt_recent);
 
 	recent_net->xt_recent = NULL;
-	spin_unlock_bh(&recent_lock);
+	mutex_unlock(&recent_mutex);
 
 	remove_proc_entry("xt_recent", net->proc_net);
 }

2.53.0

Yes, but note that this change is already part of a pending nf -> net merge request.

Dropped.

dasd_get_user_string() strips a trailing newline by looking at the last byte that was written. For an empty write that's the byte in front of the buffer it just allocated. Only look if something was written.

---
 drivers/s390/block/dasd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2ffe404b890f..e97891e58957 100644
@@ -933,7 +933,7 @@ char *dasd_get_user_string(const char __user *user_buf, size_t user_len)
 		return ERR_PTR(-EFAULT);
 	}
 	/* got the string, now strip linefeed. */
-	if (buffer[user_len - 1] == '\n')
+	if (user_len && buffer[user_len - 1] == '\n')
 		buffer[user_len - 1] = 0;
 	else
 		buffer[user_len] = 0;

2.53.0

When lcd_proc_write() and fan_proc_write() were refactored their checks were inverted but the && stayed. They only fail if sscanf() didn't parse anything and the value is out of range at the same time, and for the brightness value < 0 && value > levels can't be true at all. So an out of range brightness or fan setting is passed on to the hardware, and so is the uninitialized value if nothing could be parsed.

Restore the checks as they were before the refactoring.

---
 drivers/platform/x86/toshiba_acpi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index a0b8060836d0..9eb8f1e5aeda 100644
@@ -1498,8 +1498,8 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 	cmd[len] = '\0';
 
 	levels = dev->backlight_dev->props.max_brightness + 1;
-	if (sscanf(cmd, " brightness : %i", &value) != 1 &&
-	    value < 0 && value > levels)
+	if (sscanf(cmd, " brightness : %i", &value) != 1 ||
+	    value < 0 || value >= levels)
 		return -EINVAL;
 
 	if (set_lcd_brightness(dev, value))

@@ -1678,8 +1678,8 @@ static ssize_t fan_proc_write(struct file *file, const char __user *buf,
 		return -EFAULT;
 	cmd[len] = '\0';
 
-	if (sscanf(cmd, " force_on : %i", &value) != 1 &&
-	    value != 0 && value != 1)
+	if (sscanf(cmd, " force_on : %i", &value) != 1 ||
+	    value < 0 || value > 1)
 		return -EINVAL;
 
 	if (set_fan_status(dev, value))

2.53.0

proc_read_simdisk() looks at dev->filename without holding dev->lock. A write to the same file detaches the disk and frees the name under the lock, so a concurrent reader can copy out freed memory. Copy the name with the lock held.

---
 arch/xtensa/platforms/iss/simdisk.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

index a2ddb49c269d..7b115e023de9 100644
@@ -238,22 +238,21 @@ static ssize_t proc_read_simdisk(struct file *file, char __user *buf,
 			size_t size, loff_t *ppos)
 {
 	struct simdisk *dev = pde_data(file_inode(file));
-	const char *s = dev->filename;
-	if (s) {
-		ssize_t len = strlen(s);
-		char *temp = kmalloc(len + 2, GFP_KERNEL);
+	ssize_t len;
+	char *temp;
 
-		if (!temp)
-			return -ENOMEM;
+	/* a write sets at most a page, see proc_write_simdisk() */
+	temp = kmalloc(PAGE_SIZE + 2, GFP_KERNEL);
+	if (!temp)
+		return -ENOMEM;
 
-		len = scnprintf(temp, len + 2, "%s\n", s);
-		len = simple_read_from_buffer(buf, size, ppos,
-					      temp, len);
+	spin_lock(&dev->lock);
+	len = scnprintf(temp, PAGE_SIZE + 2, "%s\n", dev->filename ?: "");
+	spin_unlock(&dev->lock);
 
-		kfree(temp);
-		return len;
-	}
-	return simple_read_from_buffer(buf, size, ppos, "\n", 1);
+	len = simple_read_from_buffer(buf, size, ppos, temp, len);
+	kfree(temp);
+	return len;
 }
 
 static ssize_t proc_write_simdisk(struct file *file, const char __user *buf,

2.53.0

get_validate_flash_msg() adds up what snprintf() returns, which is the length of the whole string and not what fit into msg. If the update results the firmware hands back don't fit into what is left of the 256 bytes, the length ends up larger than msg and validate_flash_read() copies whatever follows it on the stack to userspace. Use scnprintf().

---
 arch/powerpc/kernel/rtas_flash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 722dbfb6fbf8..f1cd105f7426 100644
@@ -448,7 +448,7 @@ static int get_validate_flash_msg(struct rtas_validate_flash_t *args_buf,
 		n = sprintf(msg, "%d\n", args_buf->update_results);
 		if ((args_buf->update_results >= VALIDATE_CUR_UNKNOWN) ||
 		    (args_buf->update_results == VALIDATE_TMP_UPDATE))
-			n += snprintf(msg + n, msglen - n, "%s\n",
+			n += scnprintf(msg + n, msglen - n, "%s\n",
 					args_buf->buf);
 	} else {
 		n = sprintf(msg, "%d\n", args_buf->status);

2.53.0

rtas_log_read() hands out a whole slot of the error log but pSeries_log_error() only fills in the sequence number and the event, which is usually a lot shorter. The rest of the slot is whatever vmalloc() left there. Use vzalloc().

---
 arch/powerpc/kernel/rtasd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fd40864bdb70..dd0df5e164a3 100644
@@ -523,7 +523,7 @@ static int __init rtas_event_scan_init(void)
 	rtas_error_log_max = rtas_get_error_log_max();
 	rtas_error_log_buffer_max = rtas_error_log_max + sizeof(int);
 
-	rtas_log_buf = vmalloc(array_size(LOG_NUMBER,
+	rtas_log_buf = vzalloc(array_size(LOG_NUMBER,
 					  rtas_error_log_buffer_max));
 	if (!rtas_log_buf) {
 		printk(KERN_ERR "rtasd: no memory\n");

2.53.0

snd_info_entry_write() clamps the count to what is left of the entry by computing entry->size - pos as a size_t. For a position past the end that wraps and the write goes to the driver unclamped, for opl4 into sample memory outside of what the entry covers. At the end of the entry the driver gets a count of 0 and vmalloc(0) warns. Before the clamp moved into the common code the driver ignored such writes. Fail them with -ENOSPC instead, like a block device does at the end of the device. Returning 0 would make writers that use stdio retry forever.

---
 sound/core/info.c | 2 ++
 1 file changed, 2 insertions(+)

index c4d57d0809ad..982087bc8575 100644
@@ -171,6 +171,8 @@ static ssize_t snd_info_entry_write(struct file *file, const char __user *buffer
 	pos = *offset;
 	if (!valid_pos(pos, count))
 		return -EIO;
+	if (pos >= entry->size)
+		return count ? -ENOSPC : 0;
 	if (count > 0) {
 		size_t maxsize = entry->size - pos;
 		count = min(count, maxsize);

2.53.0

thanks,

Takashi

A write to /proc/scsi/BusLogic/<n> resets the statistics and returns 0. Writers that use stdio, echo for example, take that as no progress and retry forever. Return the number of bytes that were written.

---
 drivers/scsi/BusLogic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 27b446c0c5d0..7ad814f549ae 100644
@@ -3328,7 +3328,7 @@ static int blogic_write_info(struct Scsi_Host *shost, char *procbuf,
 	adapter->ext_resets = 0;
 	adapter->adapter_intern_errors = 0;
 	memset(tgt_stats, 0, BLOGIC_MAXDEV * sizeof(struct blogic_tgt_stats));
-	return 0;
+	return bytes_avail;
 }
 
 static int blogic_show_info(struct seq_file *m, struct Scsi_Host *shost)

2.53.0

ips_write_info() ignores what is written to /proc/scsi/ips/<n> and returns 0. Writers that use stdio, echo for example, take that as no progress and retry forever. Return the number of bytes that were written.

---
 drivers/scsi/ips.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 45c5b9acb664..bf3a961834fb 100644
@@ -1428,7 +1428,7 @@ ips_write_info(struct Scsi_Host *host, char *buffer, int length)
 	if (!ha)
 		return (-EINVAL);
 
-	return 0;
+	return length;
 }
 
 static int

2.53.0

debug_level_proc_write() returns the length of the string it parsed. A write that starts with a NUL byte thus returns 0 and writers that use stdio retry it forever. Return the number of bytes that were written instead, which also stops writes longer than the 11 bytes it looks at from coming up short.

---
 drivers/net/wireless/intel/ipw2x00/libipw_module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 22358b2c2ecd..552c8ff3928b 100644
@@ -236,7 +236,7 @@ static ssize_t debug_level_proc_write(struct file *file,
 	else
 		libipw_debug_level = val;
 
-	return strnlen(buf, len);
+	return count;
 }
 
 static const struct proc_ops debug_level_proc_ops = {

2.53.0

proc_scsi_host_write() copies what is written into a page and hands it to ->write_info() without terminating it. ppa and imm parse it with strncmp() and simple_strtoul(), which read on past what was written into whatever the page held before. Terminate the copy. A write is at most PROC_BLOCK_SIZE bytes, so the page always has room for that.

---
 drivers/scsi/scsi_proc.c | 1 +
 1 file changed, 1 insertion(+)

index fdc355d783da..dbd03360b5a4 100644
@@ -80,6 +80,7 @@ static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
 		ret = -EFAULT;
 		if (copy_from_user(page, buf, count))
 			goto out;
+		page[count] = '\0';
 		ret = shost->hostt->write_info(shost, page, count);
 	}
 out:

2.53.0

exitcode_proc_write() copies up to sizeof(buf) bytes of the write into buf and hands it to simple_strtol() without terminating it. So "echo -n 5" parses whatever the stack held after the '5', and a write of seven bytes or more reads past the end of buf. Terminate the copy.

---
 arch/um/kernel/exitcode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 43edc2aa57e4..0230354f4812 100644
@@ -43,9 +43,10 @@ static ssize_t exitcode_proc_write(struct file *file,
 	size_t size;
 	int tmp;
 
-	size = min(count, sizeof(buf));
+	size = min(count, sizeof(buf) - 1);
 	if (copy_from_user(buf, buffer, size))
 		return -EFAULT;
+	buf[size] = '\0';
 
 	tmp = simple_strtol(buf, &end, 0);
 	if ((*end != '\0') && !isspace(*end))

2.53.0

ntfs3_label_write() allocates as many bytes as are written with plain kmalloc(). A write that is larger than what kmalloc() can hand out then trips the page allocator's warning, which panics a panic_on_warn kernel. Use memdup_user(), which fails such an allocation quietly with -ENOMEM like before.

---
 fs/ntfs3/super.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

index 58ef6be3286b..b4479cd6bd65 100644
@@ -552,15 +552,10 @@ static ssize_t ntfs3_label_write(struct file *file, const char __user *buffer,
 	if (sb_rdonly(sb))
 		return -EROFS;
 
-	label = kmalloc(count, GFP_NOFS);
+	label = memdup_user(buffer, count);
+	if (IS_ERR(label))
+		return PTR_ERR(label);
 
-	if (!label)
-		return -ENOMEM;
-
-	if (copy_from_user(label, buffer, ret)) {
-		ret = -EFAULT;
-		goto out;
-	}
 	while (ret > 0 && label[ret - 1] == '\n')
 		ret -= 1;
 

2.53.0

__xlate_proc_name() walks the components of a name like "a/b/c" and takes whatever it finds for "a" and "b" as the directory to go on in. It never checks that they actually are directories. So proc_create("file/x") hangs x into the subdir tree of the regular file "file". Nobody can look x up there, but remove_proc_subtree() on the file removes and frees x along with it, behind the back of whoever registered x.

pktgen passes the name the writer of "add_device" picks to proc_create_data(), and only the part in front of the '@' has to name a network device:

echo "add_device lo@" > /proc/net/pktgen/kpktgend_0
echo "add_device lo@/x" > /proc/net/pktgen/kpktgend_1
echo "rem_device_all" > /proc/net/pktgen/kpktgend_0
echo "rem_device_all" > /proc/net/pktgen/kpktgend_1

The third write frees both entries and the fourth one makes kpktgend_1 call proc_remove() on the freed second one:

  BUG: KASAN: slab-use-after-free in proc_remove+0x8f/0xa0
  Read of size 8 at addr ff110001134c8e70 by task kpktgend_1/243
  Call Trace:
   proc_remove+0x8f/0xa0
   pktgen_remove_device.isra.0+0x118/0x500
   pktgen_rem_all_ifs+0xeb/0x1a0
   pktgen_thread_worker+0x1b42/0x7280

The files of a network namespace belong to the root of its user namespace and these writes don't require a capability. So once pktgen is loaded an unprivileged user can do this in namespaces of its own.

Treat a component that isn't a directory like one that doesn't exist. All names with a '/' that the tree passes in as literals only walk through directories. pktgen is fixed separately to not pass such names in the first place.

---
 fs/proc/generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2b1971da4a85..c560c96a64eb 100644
@@ -170,7 +170,7 @@ static int __xlate_proc_name(const char *name, struct proc_dir_entry **ret,
 	de = *ret ?: &proc_root;
 	while ((next = strchr(cp, '/')) != NULL) {
 		de = pde_subdir_find(de, cp, next - cp);
-		if (!de) {
+		if (!de || !S_ISDIR(de->mode)) {
 			WARN(1, "name '%s'\n", name);
 			return -ENOENT;
 		}

2.53.0

Looks good. Feel free to add:

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

"add_device" takes a name like eth0@1. Only the part in front of the '@' has to name a network device. The rest is free text and all of it becomes the name of a file in /proc/net/pktgen, where procfs takes a '/' as a directory separator. So "add_device lo@a/b" makes procfs look for a directory "lo@a" that doesn't exist and warn about it. Any user can do that in a network namespace of its own:

name 'lo@a/b'

  WARNING: fs/proc/generic.c:174 at __xlate_proc_name+0xb1/0xf0, CPU#3: bash/241
  Call Trace:
   __proc_create+0xd0/0x9b0
   proc_create_reg+0x45/0xf0
   proc_create_data+0x77/0xf0
   pktgen_thread_write+0xd8d/0x1750

With panic_on_warn that takes the machine down. And if "lo@a" exists because somebody added a device of that name, procfs used to register the new entry below that regular file, and removing "lo@a" freed both.

Reject names with a '/'.

---
 net/core/pktgen.c | 4 ++++
 1 file changed, 4 insertions(+)

index d63eeadddfcc..02af70d360bc 100644
@@ -3831,6 +3831,10 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	int err;
 	int node = cpu_to_node(t->cpu);
 
+	/* The name becomes the name of a file in /proc/net/pktgen. */
+	if (strchr(ifname, '/'))
+		return -EINVAL;
+
 	/* We don't allow a device to be on several threads */
 
 	if (__pktgen_NN_threads(t->net, ifname, FIND)) {

2.53.0

proc_register() WARNs when the directory already has an entry of the same name and fails the registration. Callers that pick their names themselves should never get there but some take names from userspace. pktgen creates a file for each device a writer of "add_device" adds, named after the device. A device can have any name, including "pgctrl" or "kpktgend_0", the names of pktgen's own files:

ip link set lo name pgctrl
ip link set pgctrl up
echo "add_device pgctrl" > /proc/net/pktgen/kpktgend_0

proc_dir_entry 'pktgen/pgctrl' already registered

  WARNING: fs/proc/generic.c:404 at proc_register+0x555/0x8b0, CPU#0: bash/241
  Call Trace:
   proc_create_data+0x9f/0xf0
   pktgen_thread_write+0xd9b/0x1780
   proc_reg_write+0x1c2/0x3c0
   vfs_write+0x1cd/0xfa0

The files of a network namespace belong to the root of its user namespace and none of this requires a capability. So once pktgen is loaded an unprivileged user can taint the kernel, or crash it with panic_on_warn, from namespaces of its own. pktgen itself copes with the failed registration just fine.

So go back to what proc_register() did before commit 6c2f91e077f1 ("proc: use WARN() rather than printk+backtrace") and what sysfs_warn_dup() does for sysfs: report the name with pr_warn() and dump the stack. While at it, do that after dropping proc_subdir_lock instead of printing a backtrace while holding the lock that every lookup and readdir in procfs needs.

---
 fs/proc/generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index c560c96a64eb..ef804cd3ec45 100644
@@ -401,9 +401,10 @@ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir,
 	write_lock(&proc_subdir_lock);
 	dp->parent = dir;
 	if (pde_subdir_insert(dir, dp) == false) {
-		WARN(1, "proc_dir_entry '%s/%s' already registered\n",
-		     dir->name, dp->name);
 		write_unlock(&proc_subdir_lock);
+		pr_warn("proc_dir_entry '%s/%s' already registered\n",
+			dir->name, dp->name);
+		dump_stack();
 		goto out_free_inum;
 	}
 	write_unlock(&proc_subdir_lock);

2.53.0

Makes sense. Feel free to add:

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

proc_readdir_de() counts the entries of a directory and resumes a listing at the number getdents64() stopped at. The entries are sorted by name, so one that is added or removed in between shifts everything behind it by one. Listing /proc/net/dev_snmp6 with a buffer that holds ".", ".." and "lo" and adding a device whose name sorts before "lo" makes the next call return "lo" a second time. Removing one that sorted before the position makes the next call skip an entry instead.

Since commit 895b4c0c79b0 ("fs/proc: fix uaf in proc_readdir_de()") an entry that is removed while it is being emitted also ends the listing right there. pde_subdir_next() finds nothing after an erased entry, so proc_readdir_de() reports the end of the directory and the next call resumes by number, skipping another entry. For /proc itself proc_root_readdir() takes that as the end of the static entries and moves on to the pids, so the rest of the static entries is missing. And if the position is behind the last entry when a call starts proc_readdir_de() returns 0, so /proc then lists no pids at all.

So remember the name of the last entry a call emitted in the file and resume at the first entry after it, like kernfs resumes after the node it stopped at. An entry that was removed while it was emitted is continued after by its name as well. If the position was moved by a seek in between the listing counts from the start like before.

---
 fs/proc/generic.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++++--------
 fs/proc/internal.h |  1 +
 fs/proc/proc_net.c |  1 +
 fs/proc/root.c     |  1 +
 4 files changed, 80 insertions(+), 12 deletions(-)

index ef804cd3ec45..47830d6af255 100644
@@ -87,6 +87,28 @@ static struct proc_dir_entry *pde_subdir_find(struct proc_dir_entry *dir,
 	return NULL;
 }
 
+/* The first entry of @dir that sorts after @name. */
+static struct proc_dir_entry *pde_subdir_after(struct proc_dir_entry *dir,
+					       const char *name,
+					       unsigned int len)
+{
+	struct rb_node *node = dir->subdir.rb_node, *after = NULL;
+
+	while (node) {
+		struct proc_dir_entry *de = rb_entry(node,
+						     struct proc_dir_entry,
+						     subdir_node);
+
+		if (proc_match(name, de, len) < 0) {
+			after = node;
+			node = node->rb_left;
+		} else {
+			node = node->rb_right;
+		}
+	}
+	return rb_entry_safe(after, struct proc_dir_entry, subdir_node);
+}
+
 static bool pde_subdir_insert(struct proc_dir_entry *dir,
 			      struct proc_dir_entry *de)
 {

@@ -277,6 +299,40 @@ struct dentry *proc_lookup(struct inode *dir, struct dentry *dentry,
 	return proc_lookup_de(dir, dentry, PDE(dir));
 }
 
+/*
+ * Where the next readdir resumes. The entries are counted, so one that is
+ * added or removed in front of the position would shift what the next call
+ * returns. The name of the last entry emitted doesn't move.
+ */
+struct proc_dir_cursor {
+	loff_t pos;			/* the position behind the entry */
+	unsigned int len;
+	char name[NAME_MAX + 1];
+};
+
+/* If this can't remember the entry the next call counts from the start. */
+static void proc_dir_cursor_set(struct file *file, struct proc_dir_entry *de,
+				loff_t pos)
+{
+	struct proc_dir_cursor *cursor = file->private_data;
+
+	if (!cursor) {
+		cursor = kmalloc(sizeof(*cursor), GFP_KERNEL);
+		if (!cursor)
+			return;
+		file->private_data = cursor;
+	}
+	cursor->pos = pos;
+	cursor->len = de->namelen;
+	memcpy(cursor->name, de->name, de->namelen);
+}
+
+int proc_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
 /*
  * This returns non-zero if at EOF, so that the /proc
  * root directory can use this and check if it should

@@ -289,23 +345,26 @@ struct dentry *proc_lookup(struct inode *dir, struct dentry *dentry,
 int proc_readdir_de(struct file *file, struct dir_context *ctx,
 		    struct proc_dir_entry *de)
 {
+	struct proc_dir_cursor *cursor = file->private_data;
+	struct proc_dir_entry *dir = de;
 	int i;
 
 	if (!dir_emit_dots(file, ctx))
 		return 0;
 
-	i = ctx->pos - 2;
 	read_lock(&proc_subdir_lock);
-	de = pde_subdir_first(de);
-	for (;;) {
-		if (!de) {
-			read_unlock(&proc_subdir_lock);
-			return 0;
-		}
-		if (!i)
-			break;
-		de = pde_subdir_next(de);
-		i--;
+	if (cursor && cursor->pos == ctx->pos) {
+		/* Resume after the last entry emitted, wherever it is now. */
+		de = pde_subdir_after(dir, cursor->name, cursor->len);
+	} else {
+		/* Moved by a seek, count from the start like always. */
+		de = pde_subdir_first(dir);
+		for (i = ctx->pos - 2; de && i; i--)
+			de = pde_subdir_next(de);
+	}
+	if (!de) {
+		read_unlock(&proc_subdir_lock);
+		return 1;
 	}
 
 	do {

@@ -318,8 +377,13 @@ int proc_readdir_de(struct file *file, struct dir_context *ctx,
 			return 0;
 		}
 		ctx->pos++;
+		proc_dir_cursor_set(file, de, ctx->pos);
 		read_lock(&proc_subdir_lock);
-		next = pde_subdir_next(de);
+		/* Removed in the meantime, go on after its name. */
+		if (RB_EMPTY_NODE(&de->subdir_node))
+			next = pde_subdir_after(dir, de->name, de->namelen);
+		else
+			next = pde_subdir_next(de);
 		pde_put(de);
 		de = next;
 	} while (de);

@@ -347,6 +411,7 @@ static const struct file_operations proc_dir_operations = {
 	.llseek			= generic_file_llseek,
 	.read			= generic_read_dir,
 	.iterate_shared		= proc_readdir,
+	.release		= proc_dir_release,
 };
 
 static int proc_net_d_revalidate(struct inode *dir, const struct qstr *name,

index df873df9afc5..04f7ce2b24ed 100644
@@ -287,6 +287,7 @@ extern struct dentry *proc_lookup(struct inode *, struct dentry *, unsigned int)
 struct dentry *proc_lookup_de(struct inode *, struct dentry *, struct proc_dir_entry *);
 extern int proc_readdir(struct file *, struct dir_context *);
 int proc_readdir_de(struct file *, struct dir_context *, struct proc_dir_entry *);
+int proc_dir_release(struct inode *, struct file *);
 
 static inline void pde_get(struct proc_dir_entry *pde)
 {

index b1f5eafb069a..dbcf87a257e1 100644
@@ -351,6 +351,7 @@ const struct file_operations proc_net_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.iterate_shared	= proc_tgid_net_readdir,
+	.release	= proc_dir_release,
 };
 
 static __net_init int proc_net_ns_init(struct net *net)

index 7fbbe92bf73a..dfee1caa26f7 100644
@@ -441,6 +441,7 @@ static const struct file_operations proc_root_operations = {
 	.read		 = generic_read_dir,
 	.iterate_shared	 = proc_root_readdir,
 	.llseek		= generic_file_llseek,
+	.release	= proc_dir_release,
 };
 
 /*

2.53.0

Looks good. Feel free to add:

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

I did consider this and didn't think it was worth it. If we can allocate a cursor we just start from the beginning.

List /proc/net/dev_snmp6 of a new network namespace while a tun device comes and goes:

- with a buffer that holds the dots and "lo", add a device whose name sorts first, finish the listing: "lo" is listed once

- with that device added and a buffer that holds the dots and it, remove it, finish the listing: "lo" is still listed

- with that device added, stall getdents64() on a userfaultfd page while it writes the device's entry, remove the device, finish the
listing: "lo" is still listed

All three fail without the fix, the first lists "lo" twice, the other two not at all.

---
 tools/testing/selftests/proc/.gitignore           |   1 +
 tools/testing/selftests/proc/Makefile             |   1 +
 tools/testing/selftests/proc/proc-readdir-churn.c | 236 ++++++++++++++++++++++
 3 files changed, 238 insertions(+)

index 9c9735570abf..8520c7878ea2 100644
@@ -8,6 +8,7 @@
 /proc-maps-race
 /proc-multiple-procfs
 /proc-net-dev-lseek
+/proc-readdir-churn
 /proc-empty-vm
 /proc-pid-vm
 /proc-self-map-files-001

index a7de2bb6d8be..e0c0d313f4d0 100644
@@ -11,6 +11,7 @@ TEST_GEN_PROGS += proc-2-is-kthread
 TEST_GEN_PROGS += proc-loadavg-001
 TEST_GEN_PROGS += proc-maps-race
 TEST_GEN_PROGS += proc-net-dev-lseek
+TEST_GEN_PROGS += proc-readdir-churn
 TEST_GEN_PROGS += proc-empty-vm
 TEST_GEN_PROGS += proc-pid-vm
 TEST_GEN_PROGS += proc-self-map-files-001

2.53.0

proc_remove() looks the entry up by its name in its parent and removes whatever it finds there:

  remove_proc_subtree(de->name, de->parent);

If the entry was removed already, say along with its parent, that is either nothing or a different entry that got the same name in the meantime, and de->parent may well be gone. Two removals of the same entry are a bug in the caller, but going by the name turns it into removing an entry that belongs to somebody else.

Since commit 895b4c0c79b0 ("fs/proc: fix uaf in proc_readdir_de()") an entry that was unlinked has an empty rb node. So check for that under proc_subdir_lock and otherwise remove the entry itself, with the loop of remove_proc_subtree() moved into a helper both of them use. Nothing can help a caller that passes an entry that has been freed already.

---
 fs/proc/generic.c | 78 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 27 deletions(-)

index 47830d6af255..23fc026d7bbb 100644
@@ -817,33 +817,14 @@ void remove_proc_entry(const char *name, struct proc_dir_entry *parent)
 }
 EXPORT_SYMBOL(remove_proc_entry);
 
-int remove_proc_subtree(const char *name, struct proc_dir_entry *parent)
+/*
+ * Remove @root, already erased from its parent, and everything below it.
+ * Called with proc_subdir_lock held for writing, returns with it dropped.
+ */
+static int __remove_proc_subtree(struct proc_dir_entry *root)
 {
-	struct proc_dir_entry *root = NULL, *de, *next;
-	const char *fn = name;
-	unsigned int len;
-
-	write_lock(&proc_subdir_lock);
-	if (__xlate_proc_name(name, &parent, &fn) != 0) {
-		write_unlock(&proc_subdir_lock);
-		return -ENOENT;
-	}
-	len = strlen(fn);
+	struct proc_dir_entry *de = root, *next;
 
-	root = pde_subdir_find(parent, fn, len);
-	if (!root) {
-		write_unlock(&proc_subdir_lock);
-		return -ENOENT;
-	}
-	if (unlikely(pde_is_permanent(root))) {
-		write_unlock(&proc_subdir_lock);
-		WARN(1, "removing permanent /proc entry '%s/%s'",
-			root->parent->name, root->name);
-		return -EINVAL;
-	}
-	pde_erase(root, parent);
-
-	de = root;
 	while (1) {
 		next = pde_subdir_first(de);
 		if (next) {

@@ -871,6 +852,34 @@ int remove_proc_subtree(const char *name, struct proc_dir_entry *parent)
 	pde_put(root);
 	return 0;
 }
+
+int remove_proc_subtree(const char *name, struct proc_dir_entry *parent)
+{
+	struct proc_dir_entry *root = NULL;
+	const char *fn = name;
+	unsigned int len;
+
+	write_lock(&proc_subdir_lock);
+	if (__xlate_proc_name(name, &parent, &fn) != 0) {
+		write_unlock(&proc_subdir_lock);
+		return -ENOENT;
+	}
+	len = strlen(fn);
+
+	root = pde_subdir_find(parent, fn, len);
+	if (!root) {
+		write_unlock(&proc_subdir_lock);
+		return -ENOENT;
+	}
+	if (unlikely(pde_is_permanent(root))) {
+		write_unlock(&proc_subdir_lock);
+		WARN(1, "removing permanent /proc entry '%s/%s'",
+			root->parent->name, root->name);
+		return -EINVAL;
+	}
+	pde_erase(root, parent);
+	return __remove_proc_subtree(root);
+}
 EXPORT_SYMBOL(remove_proc_subtree);
 
 void *proc_get_parent_data(const struct inode *inode)

@@ -882,8 +891,23 @@ EXPORT_SYMBOL_GPL(proc_get_parent_data);
 
 void proc_remove(struct proc_dir_entry *de)
 {
-	if (de)
-		remove_proc_subtree(de->name, de->parent);
+	if (!de)
+		return;
+
+	write_lock(&proc_subdir_lock);
+	/* Removed already, maybe along with its parent. */
+	if (RB_EMPTY_NODE(&de->subdir_node)) {
+		write_unlock(&proc_subdir_lock);
+		return;
+	}
+	if (unlikely(pde_is_permanent(de))) {
+		write_unlock(&proc_subdir_lock);
+		WARN(1, "removing permanent /proc entry '%s/%s'",
+		     de->parent->name, de->name);
+		return;
+	}
+	pde_erase(de, de->parent);
+	__remove_proc_subtree(de);
 }
 EXPORT_SYMBOL(proc_remove);
 

2.53.0

One nitpick below. Otherwise feel free to add:

Jan Kara <[email protected]>
SUSE Labs, CR

proc_net_ns_exit() runs after all the other pernet exits and frees net->proc_net right after removing "stat" from it. An entry that some pernet exit forgot to remove is left with a freed ->parent, and so PDE_NET() hands seq_open_net() and single_open_net() a freed struct net when the entry is opened through a file descriptor that someone kept.

There is no such entry in the tree that we know of, but nothing would tell us about one either. So warn about it like remove_proc_entry() does for a directory that isn't empty.

---
 fs/proc/proc_net.c | 6 ++++++
 1 file changed, 6 insertions(+)

index dbcf87a257e1..df5484813d5d 100644
@@ -410,7 +410,13 @@ static __net_init int proc_net_ns_init(struct net *net)
 
 static __net_exit void proc_net_ns_exit(struct net *net)
 {
+	struct proc_dir_entry *left;
+
 	remove_proc_entry("stat", net->proc_net);
+	/* Whatever is left still points at net->proc_net and so at net. */
+	left = rb_entry_safe(rb_first(&net->proc_net->subdir),
+			     struct proc_dir_entry, subdir_node);
+	WARN(left, "/proc/net: leaking at least '%s'\n", left->name);
 	pde_free(net->proc_net);
 }
 

2.53.0

OK, why not. Feel free to add:

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

Every procfs file has a ->mmap since commit 786d7e1612f0 ("Fix rmmod/read/write races in /proc entries"), whether the entry has a ->proc_mmap or not. On NOMMU validate_mmap_request() takes that to mean the file can be mapped. procfs has no ->mmap_capabilities so a regular file gets NOMMU_MAP_COPY, and a private mapping never calls ->mmap at all. Instead do_mmap() takes nommu_region_sem for writing and do_mmap_private() reads the file into the copy with kernel_read().

Reading /proc/maps takes nommu_region_sem as well:

mmap(NULL, 4096, PROT_READ, MAP_PRIVATE, open("/proc/maps", O_RDONLY), 0)

do_mmap() // down_write(&nommu_region_sem)
-> do_mmap_private()

     -> kernel_read()
        -> seq_read_iter()
           -> nommu_region_list_start()   // down_read()

The task hangs for good, holding nommu_region_sem and its own mmap_lock, and every mmap(), munmap() and exit after that hangs behind it. /proc/maps is 0444 so any user can do that.

Give the procfs files a ->mmap_capabilities on NOMMU that allows nothing. Mapping a procfs file then fails with -ENODEV, like mapping a file without ->mmap did before 786d7e1612f0. NOMMU never called ->proc_mmap() for a regular file anyway, procfs never offered NOMMU_MAP_DIRECT.

---
 fs/proc/inode.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index b7634f975d98..9933f03ab9fb 100644
@@ -466,6 +466,18 @@ proc_reg_get_unmapped_area(struct file *file, unsigned long orig_addr,
 	return rv;
 }
 
+#ifndef CONFIG_MMU
+/*
+ * NOMMU never calls ->proc_mmap() for a regular file. It reads the file into
+ * a private copy instead, with nommu_region_sem held for writing, which
+ * reading /proc/maps takes as well. Don't map procfs files at all.
+ */
+static unsigned int proc_reg_mmap_capabilities(struct file *file)
+{
+	return 0;
+}
+#endif
+
 static int proc_reg_open(struct inode *inode, struct file *file)
 {
 	struct proc_dir_entry *pde = PDE(inode);

@@ -559,6 +571,9 @@ static const struct file_operations proc_reg_file_ops = {
 	.unlocked_ioctl	= proc_reg_unlocked_ioctl,
 	.mmap		= proc_reg_mmap,
 	.get_unmapped_area = proc_reg_get_unmapped_area,
+#ifndef CONFIG_MMU
+	.mmap_capabilities = proc_reg_mmap_capabilities,
+#endif
 	.open		= proc_reg_open,
 	.release	= proc_reg_release,
 };

@@ -572,6 +587,9 @@ static const struct file_operations proc_iter_file_ops = {
 	.unlocked_ioctl	= proc_reg_unlocked_ioctl,
 	.mmap		= proc_reg_mmap,
 	.get_unmapped_area = proc_reg_get_unmapped_area,
+#ifndef CONFIG_MMU
+	.mmap_capabilities = proc_reg_mmap_capabilities,
+#endif
 	.open		= proc_reg_open,
 	.release	= proc_reg_release,
 };

@@ -586,6 +604,9 @@ static const struct file_operations proc_reg_file_ops_compat = {
 	.compat_ioctl	= proc_reg_compat_ioctl,
 	.mmap		= proc_reg_mmap,
 	.get_unmapped_area = proc_reg_get_unmapped_area,
+#ifndef CONFIG_MMU
+	.mmap_capabilities = proc_reg_mmap_capabilities,
+#endif
 	.open		= proc_reg_open,
 	.release	= proc_reg_release,
 };

@@ -600,6 +621,9 @@ static const struct file_operations proc_iter_file_ops_compat = {
 	.compat_ioctl	= proc_reg_compat_ioctl,
 	.mmap		= proc_reg_mmap,
 	.get_unmapped_area = proc_reg_get_unmapped_area,
+#ifndef CONFIG_MMU
+	.mmap_capabilities = proc_reg_mmap_capabilities,
+#endif
 	.open		= proc_reg_open,
 	.release	= proc_reg_release,
 };

2.53.0

vmcore_cleanup() has a single caller, fadump's release_mem_store() on powerpc. That checks fw_dump.dump_active without a lock and calls vmcore_cleanup() without one, and kernfs only serializes the writers of one open file. So two tasks that write to /sys/kernel/fadump/release_mem through different files run vmcore_cleanup() at the same time.

The first one removes /proc/vmcore and waits for its readers in proc_remove(). The second one finds the entry already unlinked, returns from proc_remove() right away and frees vmcore_list, the ELF headers and the device dumps while those readers still walk them. Or both of them free the same lists at once.

Serialize vmcore_cleanup() with a mutex of its own. It can't be vmcore_mutex, removing /proc/vmcore calls release_vmcore() which takes it. The second caller then finds nothing left to free.

---
 fs/proc/vmcore.c | 5 +++++
 1 file changed, 5 insertions(+)

index 1fe70c4ecc54..ccb795b85509 100644
@@ -1764,6 +1764,11 @@ fs_initcall(vmcore_init);
 /* Cleanup function for vmcore module. */
 void vmcore_cleanup(void)
 {
+	/* Not vmcore_mutex, removing /proc/vmcore calls release_vmcore(). */
+	static DEFINE_MUTEX(vmcore_cleanup_mutex);
+
+	/* Two writers of fadump's release_mem can get here at once. */
+	guard(mutex)(&vmcore_cleanup_mutex);
 	if (proc_vmcore) {
 		proc_remove(proc_vmcore);
 		proc_vmcore = NULL;

2.53.0

Looks good. Feel free to add:

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

copy_oldmem_page() maps a page of the old kernel that isn't in the linear mapping with ioremap_cache() and copies from it without checking whether the mapping worked. If it didn't, the copy reads from an offset into the NULL page. That oopses unless the copy happens to be a user copy, whose exception table entries turn the fault into a short copy.

Return -ENOMEM like the other architectures do. read_from_oldmem() turns that into -EFAULT like any other short copy.

---
 arch/powerpc/kernel/crash_dump.c | 2 ++
 1 file changed, 2 insertions(+)

index 103b6605dd68..1543917bd3e1 100644
@@ -86,6 +86,8 @@ ssize_t copy_oldmem_page(struct iov_iter *iter, unsigned long pfn,
 		csize = copy_to_iter(vaddr + offset, csize, iter);
 	} else {
 		vaddr = ioremap_cache(paddr, PAGE_SIZE);
+		if (!vaddr)
+			return -ENOMEM;
 		csize = copy_to_iter(vaddr + offset, csize, iter);
 		iounmap(vaddr);
 	}

2.53.0

A capture kernel reads the memory of the crashed kernel, and that memory may well have uncorrectable errors. On powerpc a machine check in the kernel is only recovered from if the instruction that took it has an exception table entry (cf. mce_common_process_ue()). copy_oldmem_page() copies with copy_to_iter(). For a read() of /proc/vmcore that is a user copy, all of whose loads have exception table entries, so the machine check turns into a short copy and read_from_oldmem() returns -EFAULT.

But since commit 4a22fd20379c ("vmcore: convert __read_vmcore to use an iov_iter") /proc/vmcore can be spliced too, and splice(), sendfile() and copy_file_range() copy into kernel pages with memcpy(), which has no exception table entries. A machine check there panics the capture kernel. The same goes for the reads of the ELF headers and notes that vmcore does itself.

Copy with _copy_mc_to_iter() instead. PPC64 selects ARCH_HAS_COPY_MC and there it uses copy_mc_generic() for kernel and user destinations alike, which has the exception table entries. Everywhere else it is _copy_to_iter().

---
 arch/powerpc/kernel/crash_dump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1543917bd3e1..ca7e019ae3e9 100644
@@ -83,12 +83,12 @@ ssize_t copy_oldmem_page(struct iov_iter *iter, unsigned long pfn,
 
 	if (memblock_is_region_memory(paddr, csize)) {
 		vaddr = __va(paddr);
-		csize = copy_to_iter(vaddr + offset, csize, iter);
+		csize = _copy_mc_to_iter(vaddr + offset, csize, iter);
 	} else {
 		vaddr = ioremap_cache(paddr, PAGE_SIZE);
 		if (!vaddr)
 			return -ENOMEM;
-		csize = copy_to_iter(vaddr + offset, csize, iter);
+		csize = _copy_mc_to_iter(vaddr + offset, csize, iter);
 		iounmap(vaddr);
 	}
 

2.53.0

Since commit 552d0e17ea04 ("sunrpc: convert queue_wait from global to per-cache-detail waitqueue") poll() and epoll queue their waiters for a channel file on the queue_wait head in struct cache_detail. But the cache_detail can be freed while its channel file is still open and waited on.

The procfs channel files of a network namespace are removed when the namespace goes away. Removing a procfs file waits for the callers that are in one of its methods right now and then calls ->release on every file that is still open (cf. close_pdeo()). The waiters that poll() and epoll left on the head stay queued until their file is really closed. So:

sunrpc_exit_net()
-> ip_map_cache_destroy()

     -> cache_unregister_net()
        -> remove_cache_proc_entries()
           -> proc_remove()      // ->release, the waiters stay queued
     -> cache_destroy_net()      // kfree(cd)

and when poll() returns remove_wait_queue() writes to the freed cache_detail:

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0x4b/0xa0
  Read of size 1 at addr ff110001131bb570 by task sunrpc-poll-uaf/242
  Call Trace:
   remove_wait_queue+0x22/0x1c0
   poll_freewait+0xc2/0x230
   do_sys_poll+0x9ba/0xd50
   __x64_sys_poll+0x165/0x470
  Freed by task 12:
   kfree+0x267/0x620
   sunrpc_exit_net+0x34/0x80
   ops_undo_list+0x271/0x8d0
   cleanup_net+0x3f3/0x860

Closing an epoll instance that watches the file gets there through ep_unregister_pollwait() instead.

The channel files of a network namespace belong to the root of its user namespace and cache_open() doesn't check for a capability. So an unprivileged user can open the channel file of a namespace of its own from the outside, poll it and let the namespace die.

Go back to a single static wait queue that never goes away. A wakeup for one cache wakes the waiters of all caches again but cache_poll() reports per cache_detail so that's just a spurious wakeup. procfs should really own the wait queue of its files like kernfs does but that's nothing for stable.

---
 include/linux/sunrpc/cache.h |  1 -
 net/sunrpc/cache.c           | 12 +++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

index 2735c332ddb7..bf4fc543e1e5 100644
@@ -119,7 +119,6 @@ struct cache_detail {
 	struct list_head	requests;
 	struct list_head	readers;
 	spinlock_t		queue_lock;
-	wait_queue_head_t	queue_wait;
 	u64			next_seqno;
 
 	atomic_t		writers;		/* how many time is /channel open */

index 1bc04109d213..1ceaea7e01b9 100644
@@ -404,7 +404,6 @@ void sunrpc_init_cache_detail(struct cache_detail *cd)
 	INIT_LIST_HEAD(&cd->requests);
 	INIT_LIST_HEAD(&cd->readers);
 	spin_lock_init(&cd->queue_lock);
-	init_waitqueue_head(&cd->queue_wait);
 	cd->next_seqno = 1;
 	spin_lock(&cache_list_lock);
 	cd->nextcheck = 0;

@@ -965,13 +964,20 @@ static ssize_t cache_write(struct file *filp, const char __user *buf,
 	return ret;
 }
 
+/*
+ * Not in struct cache_detail: poll and epoll waiters stay queued until
+ * their file is closed, but a channel file in procfs can stay open after
+ * the cache_detail of its dying network namespace has been freed.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(queue_wait);
+
 static __poll_t cache_poll(struct file *filp, poll_table *wait,
 			       struct cache_detail *cd)
 {
 	__poll_t mask;
 	struct cache_reader *rp = filp->private_data;
 
-	poll_wait(filp, &cd->queue_wait, wait);
+	poll_wait(filp, &queue_wait, wait);
 
 	/* alway allow write */
 	mask = EPOLLOUT | EPOLLWRNORM;

@@ -1236,7 +1242,7 @@ static int cache_do_upcall(struct cache_detail *detail, struct cache_head *h)
 	spin_unlock(&detail->queue_lock);
 	if (ret != -EAGAIN && detail->cache_notify)
 		detail->cache_notify(detail, h);
-	wake_up(&detail->queue_wait);
+	wake_up(&queue_wait);
 	if (ret == -EAGAIN) {
 		kfree(buf);
 		kfree(crq);

2.53.0

Fwiw, I have a patch series on top of this that makes procfs own the waitqueue and then this whole UAF issues goes away structurally and you get to elide that global thing here.

esas2r_ioctl_handler() copies in a struct atto_express_ioctl, a bit over 512k, and hands the union member for the command to the handlers. For EXPRESS_IOCTL_HBA and EXPRESS_IOCTL_SMP the length of the buffered ioctl is the size of the member plus lengths that come straight from userspace:

  bi.length = sizeof(struct atto_ioctl) + ioctl_hba->data_length;

bi.length = sizeof(struct atto_ioctl_smp)
+ le32_to_cpu(si->req_length)
+ le32_to_cpu(si->rsp_length);

handle_buffered_ioctl() copies bi.length bytes from the member into a DMA buffer and, once the adapter is done, copies the DMA buffer back over the member. Nothing checks that the lengths fit into the struct that was copied in. So a large data_length makes it read past the struct and then write the DMA buffer back over the memory behind it.

The firmware commands pass sizeof(struct atto_express_ioctl) to esas2r_{read,write}_{fw,fs}() as the length of data.fwrw.image, which starts 28 bytes into the struct. So they can read and write 28 bytes past it as well.

The handler is reachable through /proc/scsi/esas2r/ATTONode. It is created with mode 0, which procfs turns into 0444, and esas2r doesn't check for a capability anywhere. So any local user can do this on a machine with such an adapter.

Bound all of these by the room from the member to the end of the struct. read_hw() passes handle_hba_ioctl() a single struct atto_ioctl and only ever returns that, so there the data length has to be zero.

---
 drivers/scsi/esas2r/esas2r.h       |  2 +-
 drivers/scsi/esas2r/esas2r_ioctl.c | 41 ++++++++++++++++++++++++--------------
 drivers/scsi/esas2r/esas2r_main.c  |  3 ++-
 3 files changed, 29 insertions(+), 17 deletions(-)

index a763edd05fe4..10101f50b282 100644
@@ -967,7 +967,7 @@ int esas2r_write_params(struct esas2r_adapter *a, struct esas2r_request *rq,
 int esas2r_ioctl_handler(void *hostdata, unsigned int cmd, void __user *arg);
 int esas2r_ioctl(struct scsi_device *dev, unsigned int cmd, void __user *arg);
 u8 handle_hba_ioctl(struct esas2r_adapter *a,
-		    struct atto_ioctl *ioctl_hba);
+		    struct atto_ioctl *ioctl_hba, size_t room);
 enum scsi_qc_status esas2r_queuecommand(struct Scsi_Host *host,
 					struct scsi_cmnd *cmd);
 int esas2r_show_info(struct seq_file *m, struct Scsi_Host *sh);

index ada278c24c51..75a70ae9441b 100644
@@ -178,7 +178,8 @@ static ssize_t read_hw(struct file *file, struct kobject *kobj,
 	if (!a->local_atto_ioctl)
 		return -ENOMEM;
 
-	if (handle_hba_ioctl(a, a->local_atto_ioctl) != IOCTL_SUCCESS)
+	if (handle_hba_ioctl(a, a->local_atto_ioctl,
+			     sizeof(struct atto_ioctl)) != IOCTL_SUCCESS)
 		return -ENOMEM;
 
 	memcpy(buf, a->local_atto_ioctl, length);

2.53.0

This whole codepath as currently implemented is structurally fscked. So it's not something fixable in this series apart from the glaringly obvious.

But for the record:

- The ATTONode entry thing is created as a side effect of reading another file.

So esas2r_info() registers a character device and creates /proc/scsi/esas2r/ATTONode the first time it runs. That pins a global pointer to whichever host triggered it. So the control node only exists after someone looked at the host info. So it is bound to the first host and not to the adapter it operates on.

The lifeteime is then tied to the module and not to the host. The entry is removed in esas2r_exit() on module exit.

But the directory it lives in belongs to SCSI. That removes it when the last host of the template goes away.

So unbind or hotunplug of the last adapter removes a non-empty directory... the dir is unlinked and the ATTONode leaks and stays reachable through a cached dentry.

And the global host pointer dangles.

So every ioctl on the node dereferences it.

It's like non-empty directory removal basically. Fun.

- No privilege check anywhere. The node is created with mode 0 so 0444 with procfs the the driver never calls capable().

Hence, any local user can read and write firmware and NVRAM and issue VDA commands.

- The ioctl protocol itself (where to begin):
- Plain-number commands
- one struct copied in whole
- unbounded lengths taken straight from userspace (well, that's fixed here)

Sorry, I'm not going to redesign esas2r.
And from what I can tell that ATTO stuff is legacy anyway so good riddance as far as I'm concerned.

The commands for /proc/powerpc/ofdt that carry properties, "add_node", "add_property" and "update_property", can make the parser read past the buffer they were copied into:

(1) do_update_property() computes the end of the buffer after parse_node() moved buf past the phandle. So the end is off by the length of the phandle string, which can be padded with zeroes, and parse_next_property() accepts a value that reaches that far past the command. new_property() then copies those bytes into the property, which everyone can read in /proc/device-tree.

(2) parse_next_property() parses the length with simple_strtoul() into an int and only rejects -1. A length above INT_MAX turns negative, tmp += *length moves backwards and passes the check against the end, so *tmp is read up to 2G in front of the buffer.

(3) Updating "slb-size" or "ibm,slb-size" reads an int from the value whatever its length.

Compute the end before parsing the phandle, reject lengths that don't fit into an int or into what is left of the buffer, and only take the slb size from a value that holds one. The file is root-only and refused under lockdown but the parser still shouldn't read past its input.

---
 arch/powerpc/platforms/pseries/reconfig.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

index 18e3f1a036e3..b41c5db726c9 100644
@@ -114,6 +114,7 @@ static void release_prop_list(const struct property *prop)
 static char * parse_next_property(char *buf, char *end, char **name, int *length,
 				  unsigned char **value)
 {
+	unsigned long len;
 	char *tmp;
 
 	*name = buf;

@@ -133,13 +134,13 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
 	}
 
 	/* now we're on the length */
-	*length = -1;
-	*length = simple_strtoul(tmp, &tmp, 10);
-	if (*length == -1) {
+	len = simple_strtoul(tmp, &tmp, 10);
+	if (len > INT_MAX) {
 		printk(KERN_ERR "property parse failed in %s at line %d\n",
 		       __func__, __LINE__);
 		return NULL;
 	}
+	*length = len;
 	if (*tmp != ' ' || ++tmp >= end) {
 		printk(KERN_ERR "property parse failed in %s at line %d\n",
 		       __func__, __LINE__);

@@ -148,13 +149,13 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
 
 	/* now we're on the value */
 	*value = tmp;
-	tmp += *length;
-	if (tmp > end) {
+	if (len > end - tmp) {
 		printk(KERN_ERR "property parse failed in %s at line %d\n",
 		       __func__, __LINE__);
 		return NULL;
 	}
-	else if (tmp < end && *tmp != ' ' && *tmp != '\0') {
+	tmp += len;
+	if (tmp < end && *tmp != ' ' && *tmp != '\0') {
 		printk(KERN_ERR "property parse failed in %s at line %d\n",
 		       __func__, __LINE__);
 		return NULL;

@@ -320,8 +321,8 @@ static int do_update_property(char *buf, size_t bufsize)
 	char *name, *end, *next_prop;
 	int length;
 	struct property *newprop;
-	buf = parse_node(buf, bufsize, &np);
 	end = buf + bufsize;
+	buf = parse_node(buf, bufsize, &np);
 
 	if (!np)
 		return -ENODEV;

@@ -337,7 +338,8 @@ static int do_update_property(char *buf, size_t bufsize)
 	if (!newprop)
 		return -ENOMEM;
 
-	if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
+	if ((!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size")) &&
+	    length >= (int)sizeof(int))
 		slb_set_size(*(int *)value);
 
 	return of_update_property(np, newprop);

2.53.0

h_get_ppp() returns the status of the H_GET_PPP hcall as an unsigned int. update_ppp() hands it on as its ssize_t result, and lparcfg_write() turns the hcall statuses it knows into an errno and returns anything else as it is. So when H_GET_PPP fails, say with H_PARAMETER (-4), the status comes back as 4294967292 and a write to /proc/powerpc/lparcfg reports that many bytes written.

Return the status as the long it is. lparcfg_write() then treats it like the status of H_SET_PPP and an unknown one is at least negative. Before commit 11529396ea31 ("powerpc/pseries: Split processor entitlement retrieval and gathering to helper routines") a failed H_GET_PPP turned into -EIO.

---
 arch/powerpc/platforms/pseries/lparcfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3280b7968cab..aa9b5b141434 100644
@@ -113,7 +113,7 @@ struct hvcall_ppp_data {
  *	          XXXXXX - Entitled procs capacity % available to the
  *			   partitions pool.
  */
-static unsigned int h_get_ppp(struct hvcall_ppp_data *ppp_data)
+static long h_get_ppp(struct hvcall_ppp_data *ppp_data)
 {
 	unsigned long retbuf[PLPAR_HCALL9_BUFSIZE] = {0};
 	long rc;

2.53.0

proc_sys_readdir() counts the entries of a directory and resumes a listing at the number getdents64() stopped at. The entries are sorted by name, so one that is registered or unregistered in between shifts everything behind it by one. Listing /proc/sys/net/ipv4/conf with a buffer that holds ".", ".." and "all" and registering a device whose name sorts before "all" makes the next call return "all" a second time. Unregistering one that sorted before the position makes it skip an entry instead. So a listing that takes more than one call shows a name twice, or not at all. Even the 64k buffer of ls needs a second call to find the end of the directory, so a device that comes up right then repeats the last entry.

So remember the name of the last entry a call emitted in the file and resume at the first usable entry after it, like kernfs resumes after the node it stopped at. The entry itself can't be pinned across calls, unregistering waits for that. If the position was moved by a seek in between the listing counts from the start like before, seeking to an arbitrary position has never been better than that.

That also takes one walk down the tree to resume instead of a walk over every entry in front of the position with a round trip through sysctl_lock for each of them.

---
 fs/proc/proc_sysctl.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 3 deletions(-)

index 6c0859c66563..38b61887b59c 100644
@@ -417,6 +417,54 @@ static void next_entry(struct ctl_table_header **phead, const struct ctl_table *
 	*pentry = entry;
 }
 
+/*
+ * The first usable entry of @dir after @name, or the first one at all for a
+ * NULL @name. The entries are sorted by name, so that is where a listing
+ * resumes when entries in front of it came or went in between. -ENOENT if
+ * the directory is being unregistered.
+ */
+static int entry_after(struct ctl_dir *dir, const char *name, int namelen,
+		       struct ctl_table_header **phead,
+		       const struct ctl_table **pentry)
+{
+	struct ctl_table_header *head = NULL;
+	const struct ctl_table *entry = NULL;
+	struct rb_node *node, *after = NULL;
+	struct ctl_node *ctl_node;
+
+	spin_lock(&sysctl_lock);
+	if (unlikely(dir->header.unregistering)) {
+		spin_unlock(&sysctl_lock);
+		return -ENOENT;
+	}
+	if (!name) {
+		after = rb_first(&dir->root);
+	} else for (node = dir->root.rb_node; node;) {
+		const struct ctl_table *table;
+		struct ctl_table_header *h;
+
+		ctl_node = rb_entry(node, struct ctl_node, node);
+		h = ctl_node->header;
+		table = &h->ctl_table[ctl_node - h->node];
+		if (namecmp(table->procname, strlen(table->procname),
+			    name, namelen) > 0) {
+			after = node;
+			node = node->rb_left;
+		} else {
+			node = node->rb_right;
+		}
+	}
+	ctl_node = first_usable_entry(after);
+	spin_unlock(&sysctl_lock);
+	if (ctl_node) {
+		head = ctl_node->header;
+		entry = &head->ctl_table[ctl_node - head->node];
+	}
+	*phead = head;
+	*pentry = entry;
+	return 0;
+}
+
 /*
  * sysctl_perm does NOT grant the superuser all rights automatically, because
  * some sysctl variables are readonly even to root.

@@ -675,6 +723,36 @@ static __poll_t proc_sys_poll(struct file *filp, poll_table *wait)
 	return ret;
 }
 
+/*
+ * Where the next readdir resumes. The entries are counted, so one that is
+ * registered or unregistered in front of the position would shift what
+ * the next call returns. The name of the last entry emitted doesn't move.
+ */
+struct sysctl_cursor {
+	loff_t pos;			/* the position behind the entry */
+	int len;
+	char name[NAME_MAX + 1];
+};
+
+/* If this can't remember the entry the next call counts from the start. */
+static void sysctl_cursor_set(struct file *file, const char *name, int len,
+			      loff_t pos)
+{
+	struct sysctl_cursor *cursor = file->private_data;
+
+	if (len > NAME_MAX)
+		return;
+	if (!cursor) {
+		cursor = kmalloc(sizeof(*cursor), GFP_KERNEL);
+		if (!cursor)
+			return;
+		file->private_data = cursor;
+	}
+	cursor->pos = pos;
+	cursor->len = len;
+	memcpy(cursor->name, name, len);
+}
+
 static bool proc_sys_fill_cache(struct file *file,
 				struct dir_context *ctx,
 				struct ctl_table_header *head,

@@ -753,8 +831,11 @@ static int scan(struct ctl_table_header *head, const struct ctl_table *table,
 	else
 		res = proc_sys_fill_cache(file, ctx, head, table);
 
-	if (res)
+	if (res) {
 		ctx->pos = *pos;
+		sysctl_cursor_set(file, table->procname, strlen(table->procname),
+				  ctx->pos);
+	}
 
 	return res;
 }

@@ -762,6 +843,7 @@ static int scan(struct ctl_table_header *head, const struct ctl_table *table,
 static int proc_sys_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct ctl_table_header *head = grab_header(file_inode(file));
+	struct sysctl_cursor *cursor = file->private_data;
 	struct ctl_table_header *h = NULL;
 	const struct ctl_table *entry;
 	struct ctl_dir *ctl_dir;

@@ -775,9 +857,20 @@ static int proc_sys_readdir(struct file *file, struct dir_context *ctx)
 	if (!dir_emit_dots(file, ctx))
 		goto out;
 
-	pos = 2;
+	/*
+	 * Resume after the last entry the previous call emitted. Only a seek
+	 * that moved the position away from it counts from the start, like
+	 * seeking to an arbitrary position always did.
+	 */
+	if (cursor && cursor->pos == ctx->pos) {
+		entry_after(ctl_dir, cursor->name, cursor->len, &h, &entry);
+		pos = ctx->pos;
+	} else {
+		first_entry(ctl_dir, &h, &entry);
+		pos = 2;
+	}
 
-	for (first_entry(ctl_dir, &h, &entry); h; next_entry(&h, &entry)) {
+	for (; h; next_entry(&h, &entry)) {
 		if (!scan(h, entry, &pos, file, ctx)) {
 			sysctl_head_finish(h);
 			break;

@@ -788,6 +881,12 @@ static int proc_sys_readdir(struct file *file, struct dir_context *ctx)
 	return 0;
 }
 
+static int proc_sys_dir_release(struct inode *inode, struct file *file)
+{
+	kfree(file->private_data);
+	return 0;
+}
+
 static int proc_sys_permission(const struct mnt_idmap *idmap,
 			       struct inode *inode, int mask)
 {

@@ -867,6 +966,7 @@ static const struct file_operations proc_sys_dir_file_operations = {
 	.read		= generic_read_dir,
 	.iterate_shared	= proc_sys_readdir,
 	.llseek		= generic_file_llseek,
+	.release	= proc_sys_dir_release,
 };
 
 static const struct inode_operations proc_sys_inode_operations = {

2.53.0

Just one coding style nit below. Otherwise feel free to add:

	} else {
		for (...) {

								Honza

Jan Kara <[email protected]>
SUSE Labs, CR