net/ethernet/eth.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
From: Ozgur Karatas <ozgur@goosey.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.
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.
This patch fixes the issue in nvmem_get_mac_address() where invalid
mac addresses could be read due to improper error handling.
Signed-off-by: Ozgur Karatas <ozgur@goosey.org>
---
net/ethernet/eth.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 4e3651101b86..1c5649b956e9 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -549,12 +549,12 @@ int nvmem_get_mac_address(struct device *dev,
void *addrbuf)
return PTR_ERR(mac);
if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
- kfree(mac);
+ kfree((void *)mac);
return -EINVAL;
}
ether_addr_copy(addrbuf, mac);
- kfree(mac);
+ kfree((void *)mac);
return 0;
}
@@ -565,11 +565,16 @@ static int fwnode_get_mac_addr(struct
fwnode_handle *fwnode,
int ret;
ret = fwnode_property_read_u8_array(fwnode, name, addr, ETH_ALEN);
- if (ret)
+ if (ret) {
+ pr_err("Failed to read MAC address property %s\n", name);
return ret;
+ }
- if (!is_valid_ether_addr(addr))
+ if (!is_valid_ether_addr(addr)) {
+ pr_err("Invalid MAC address read for %s\n", name);
return -EINVAL;
+ }
+
return 0;
}
--
2.39.5On Thu, May 08, 2025 at 02:14:00AM +0000, Ozgur Kara wrote:
> From: Ozgur Karatas <ozgur@goosey.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.
>
> 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.
What warning do you see from the compiler?
> @@ -565,11 +565,16 @@ static int fwnode_get_mac_addr(struct
> fwnode_handle *fwnode,
> int ret;
>
> ret = fwnode_property_read_u8_array(fwnode, name, addr, ETH_ALEN);
> - if (ret)
> + if (ret) {
> + pr_err("Failed to read MAC address property %s\n", name);
> return ret;
> + }
>
> - if (!is_valid_ether_addr(addr))
> + if (!is_valid_ether_addr(addr)) {
> + pr_err("Invalid MAC address read for %s\n", name);
> return -EINVAL;
> + }
> +
> return 0;
> }
Look at how it is used:
int of_get_mac_address(struct device_node *np, u8 *addr)
{
int ret;
if (!np)
return -ENODEV;
ret = of_get_mac_addr(np, "mac-address", addr);
if (!ret)
return 0;
ret = of_get_mac_addr(np, "local-mac-address", addr);
if (!ret)
return 0;
ret = of_get_mac_addr(np, "address", addr);
if (!ret)
return 0;
return of_get_mac_address_nvmem(np, addr);
}
We keep trying until we find a MAC address. It is not an error if a
source does not have a MAC address, we just keep going and try the
next.
So you should not print an message if the property does not
exist. Other errors, EIO, EINVAL, etc, are O.K. to print a warning.
Andrew
---
pw-bot: cr
Andrew Lunn <andrew@lunn.ch>, 8 May 2025 Per, 15:01 tarihinde şunu yazdı:
>
> On Thu, May 08, 2025 at 02:14:00AM +0000, Ozgur Kara wrote:
> > From: Ozgur Karatas <ozgur@goosey.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.
> >
> > 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.
>
> What warning do you see from the compiler?
Hello Andrew,
My compiler didnt give an error to this but we had to declare that
pointer would be used as a memory block not data and i added (void *)
because i was hoping that mac variable would use it to safely remove
const so expect a parameter of type void * avoid possible compiler
incompatibilities.
I guess, however if mac is a pointer of a different type (i guess) we
use kfree(mac) without converting it to (void *) type compiler may
give an error.
For example will give error:
int mac = 10;
kfree(mac);
because pointer was of a type incompatible with const void * and i
think its not a compiler error, in this case it could be an error at
runtime bug and type of error could turn into a memory leak.
for example use clang i guess give error warning passing argument 1 of
kfree qualifier from pointer target type.
am i thinking wrong?
> > @@ -565,11 +565,16 @@ static int fwnode_get_mac_addr(struct
> > fwnode_handle *fwnode,
> > int ret;
> >
> > ret = fwnode_property_read_u8_array(fwnode, name, addr, ETH_ALEN);
> > - if (ret)
> > + if (ret) {
> > + pr_err("Failed to read MAC address property %s\n", name);
> > return ret;
> > + }
> >
> > - if (!is_valid_ether_addr(addr))
> > + if (!is_valid_ether_addr(addr)) {
> > + pr_err("Invalid MAC address read for %s\n", name);
> > return -EINVAL;
> > + }
> > +
> > return 0;
> > }
>
> Look at how it is used:
>
> int of_get_mac_address(struct device_node *np, u8 *addr)
> {
> int ret;
>
> if (!np)
> return -ENODEV;
>
> ret = of_get_mac_addr(np, "mac-address", addr);
> if (!ret)
> return 0;
>
> ret = of_get_mac_addr(np, "local-mac-address", addr);
> if (!ret)
> return 0;
>
> ret = of_get_mac_addr(np, "address", addr);
> if (!ret)
> return 0;
>
> return of_get_mac_address_nvmem(np, addr);
> }
>
> We keep trying until we find a MAC address. It is not an error if a
> source does not have a MAC address, we just keep going and try the
> next.
>
> So you should not print an message if the property does not
> exist. Other errors, EIO, EINVAL, etc, are O.K. to print a warning.
>
ah, i understand that its already checked continuously via a loop so
it would be unnecessary if i printed an error message for
of_get_mac_addr.
hm this is an expected situation and device are just moving on to the
next property I understand thank you.
I will look at code again and understand it better.
Thanks for help,
Regards
Ozgur
> Andrew
>
> ---
> pw-bot: cr
>
>
© 2016 - 2025 Red Hat, Inc.