linux-kernel - Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac

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>] [day] [month] [year] [list]

Message-ID: <20250509173014.GQ3339421@horms.kernel.org>
Date: Fri, 9 May 2025 18:30:14 +0100
From: Simon Horman <horms@...nel.org>
To: Ozgur Kara <ozgur@...sey.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address()
 where invalid mac addresses

On Thu, May 08, 2025 at 02:14:00AM +0000, Ozgur Kara wrote:
> From: Ozgur Karatas <ozgur@...sey.org>
> 
> it's necessary to log error returned from
> fwnode_property_read_u8_array because there is no detailed information
> when addr returns an invalid mac address.

Maybe "useful" would be better wording than "necessary".
Logging doesn't seem to be a hard requirement to me.

Moreover, I'm not sure that logging is appropriate.
E.g. fwnode_get_mac_addr() is called by fwnode_get_mac_address(),
which is in turn called by acpi_get_mac_address(), which logs if call
fails.

> kfree(mac) should actually be marked as kfree((void *)mac) because mac
> pointer is of type const void * and type conversion is required so
> data returned from nvmem_cell_read() is of same type.

It seems to me that nvmem_cell_read returns void *.
So a good approach would be to change type of mac to void *.
In any case I don't think casting away const is the right approach;
what is the point of const if it is selectively ignored?

Also, I think this should be a separate patch to the logging change:
one patch per issue. If there is more than one patch then I would
suggest collecting them together in a patch-set with a cover letter.

> 
> This patch fixes the issue in nvmem_get_mac_address() where invalid
> mac addresses could be read due to improper error handling.

I don't see any changes to the program flow for error handling in this patch.
It doesn't seem like a fix to me.

> 
> Signed-off-by: Ozgur Karatas <ozgur@...sey.org>

-- 
pw-bot: changes-requested