On Fri, Dec 30, 2022 at 1:31 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > On Tue, Dec 27, 2022 at 7:34 AM Hang Zhang <zh.nvgt@xxxxxxxxx> wrote: > > > > cm_write() is the .write callback of the custom_method debugfs > > interface, it operates on a global pointer "buf" (e.g., dereference, > > allocate, free, and nullification), the problem is that cm_write() > > is not protected by any locks, so concurrent invocations of it > > may cause use-after-free issues for "buf", e.g., one invocation > > may have just freed "buf" while being preempted before nullifying > > the pointer, then another invocation can dereference the now dangling > > "buf" pointer. > > > > Fix the issue by protecting the "buf" operations in cm_write() with > > the inode write lock. Note that the .llseek callback of the debugfs > > interface has been protected by the same lock, this patch basically > > introduces it to the .write callback as well. > > The problem is there, but the whole state is not protected from > concurrent use and the fix doesn't look sufficient to me (for example, > a different writer may start writing into the file before the previous > one has finished and the result will still be broken AFAICS). > > It looks like the file should be prevented from being opened by more > than one writer at a time. > > Or maybe it's time to drop this interface from the kernel altogether. Hi, Rafael, Thank you very much for your feedback! We initially intended to bring up this potential concurrent UAF issue to the community with this tentative patch, but we do not have deep domain knowledge for the ACPI subsystem and the bigger picture, so your comment is highly valuable to us! As far as I can understand, inode_lock is uniquely associated with the opened file, e.g., if two writers open the same debugfs file and write to it, then inode_lock as used in this patch should be able to synchronize their concurrent write because their inode_lock are on the same semaphore. Do you mean that this "custom_method" driver will handle the .open/.write of multiple different debugfs file instances, so that writers accessing different file instances will not be properly synchronized with inode_lock? Sorry if I missed anything here, and thank you in advance for your explanation! I also think it is a good solution to totally drop this interface if the maintainers consider it appropriate (I do not have the knowledge to assess the role this interface plays, though). Thank you for your help!
