linux-kernel - Re: [PATCH v4 00/10] riscv: Allow userspace to directly access perf counters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4761144.fSbbhVQpq0@basile.remlab.net>
Date:   Tue, 18 Jul 2023 20:06:36 +0300
From:   Rémi Denis-Courmont <remi@...lab.net>
To:     Atish Patra <atishp@...shpatra.org>,
        linux-riscv@...ts.infradead.org
Cc:     linux-kernel@...r.kernel.org,
        Aurelien Jarno <aurelien@...el32.net>,
        Mathieu Malaterre <malat@...ian.org>,
        Jan Newger <jannewger@...gle.com>
Subject: Re: [PATCH v4 00/10] riscv: Allow userspace to directly access perf counters

	Hi,

Le tiistaina 18. heinäkuuta 2023, 2.22.54 EEST Atish Patra a écrit :
> > AFAIK, if the default settings breaks user space, the patchset is
> > considered to break user space. That being the case, either this case is
> > deemed special enough that breaking user space is OK, or it is not.

> This case is a special case as the usage was incorrect in the first
> place.

I agree that it's not only insecure but also incorrect. However it mostly 
works. In fact I don't disagree with the change as such, but I think that the 
commit messages are misleading and confusing. For a start, in one place it 
says that it is not breaking user space and in another it says basically the 
opposite.

(Unfortunately, not everybody agrees with the change. I can't seem to get 
FFmpeg's checkasm tool fixed:
http://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/312245.html )

Also this is not the first time somebody argues that an API should be removed 
because it's broken. That's not special.

> Any application that genuinely requires rdcycle can always get
> it now via the perf interface.

Sure. But the question is whether it breaks user space and if so, whether 
that's acceptable. Existing apps that call RDCYCLE will now fail, presumbly 
receive SIGILL(?).

> If the insecure and incorrect behavior is allowed, we suspect the user
> space behavior will never be fixed as it is hard to put a future flag
> date in these cases.

For better or worse, I can only agree there. But then adding an option to 
preserve the broken behaviour is begging for people to (ab)use it, and indeed 
never fix the problem, and never be able to remove the option.

> > If it is not OK, then the only way out that I can think of, consists of
> > trapping and emulating the counters, returning the same sanitised values
> > that Linux perf would return. Then you can add a kernel config option to
> > disable that trap-and-emulation code in the future.
> What do you mean by "sanitised" value ?

I mean whatever avoids creating a security issue. Presumably report the number 
of cycles spent in the calling thread and in user mode since the first time 
that the process called RDCYCLE?

Maybe it's not reasonable for complexity or performance reasons, but then IMO, 
it deserves a little bit better explaining in the commit message.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/