On Thu, Nov 04, 2021 at 12:35, "Russell King (Oracle)" [off-list ref] wrote:
On Thu, Nov 04, 2021 at 12:17:47PM +0100, Tobias Waldekranz wrote:
quoted
On Wed, Nov 03, 2021 at 20:36, Andrew Lunn [off-list ref] wrote:
quoted
On Wed, Nov 03, 2021 at 08:42:07PM +0200, Grygorii Strashko wrote:
quoted
On 03/11/2021 02:27, Andrew Lunn wrote:
quoted
quoted
quoted
What i find interesting is that you and the other resent requester are
using the same user space tool. If you implement C45 over C22 in that
tool, you get your solution, and it will work for older kernels as
well. Also, given the diverse implementations of this IOTCL, it
probably works for more drivers than just those using phy_mii_ioctl().
Do you mean change uapi, like
add mdio_phy_id_is_c45_over_c22() and
flag #define MDIO_PHY_ID_C45_OVER_C22 0x4000?
No, i mean user space implements C45 over C22. Make phytool write
MII_MMD_CTRL and MII_MMD_DATA to perform a C45 over C22.
Now I give up - as mentioned there is now way to sync User space vs Kernel
MMD transactions and so no way to get trusted results.
Except that there is a way: https://github.com/wkz/mdio-tools
I'm guessing that this hasn't had much in the way of review, as it has
a nice exploitable bug - you really want "pc" to be unsigned in
mdio_nl_eval(), otherwise one can write a branch instruction that makes
"pc" negative.
You are quite right, it never got that far as it was NAKed on principle
before that. I welcome the review, this is one of the reasons why I
would love to have it in mainline. Alternatively, if someone has a
better idea, I wouldn't mind adapting mdio-tools to whatever that
interface would be.
I agree that there should be much more rigorous checks around the
modification of the PC. I will get on that.
Also it looks like one can easily exploit this to trigger any of your
BUG_ON()/BUG() statements, thereby crashing while holding the MDIO bus
lock causing a denial of service attack.
The idea is that this is pre-validated in mdio_nl_validate_insn. Each
instruction lists their acceptable argument types in mdio_nl_op_protos.
I also see nothing that protects against any user on a system being
able to use this interface, so the exploits above can be triggered by
any user. Moreover, this lack of protection means any user on the
system can use this interface to write to a PHY.
I was under the impression that specifying GENL_ADMIN_PERM in the
`struct genl_ops` would require the caller to hold CAP_NET_ADMIN?
Given that some PHYs today contain firmware, this gives anyone access
to reprogram the PHY firmware, possibly introducing malicious firmware.
I hope no one is using this module in a production environment.
Thanks for your review.