Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Simon Horman]

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.

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@goosey.org>

-- 
pw-bot: changes-requested

[PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Ozgur Kara]

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.5

Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Andrew Lunn]

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?

> @@ -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

Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Ozgur Kara]

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
>
>

Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Andrew Lunn]

On Thu, May 08, 2025 at 12:37:40PM +0000, Ozgur Kara wrote:
> 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.

/**
 * kfree - free previously allocated memory
 * @object: pointer returned by kmalloc() or kmem_cache_alloc()
 *
 * If @object is NULL, no operation is performed.
 */
void kfree(const void *object)
{

So kfree() expects a const void *.

int nvmem_get_mac_address(struct device *dev, void *addrbuf)
{
	struct nvmem_cell *cell;
	const void *mac;

mac is a const void *

In general, casts should not be used, the indicate bad design. But the
cast you are adding appears to be wrong, which is even worse.

	Andrew

Re: [PATCH] net: ethernet: Fixe issue in nvmem_get_mac_address() where invalid mac addresses

[Ozgur Kara]

Andrew Lunn <andrew@lunn.ch>, 8 May 2025 Per, 16:49 tarihinde şunu yazdı:
>
> On Thu, May 08, 2025 at 12:37:40PM +0000, Ozgur Kara wrote:
> > 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.
>
> /**
>  * kfree - free previously allocated memory
>  * @object: pointer returned by kmalloc() or kmem_cache_alloc()
>  *
>  * If @object is NULL, no operation is performed.
>  */
> void kfree(const void *object)
> {
>
> So kfree() expects a const void *.
>
> int nvmem_get_mac_address(struct device *dev, void *addrbuf)
> {
>         struct nvmem_cell *cell;
>         const void *mac;
>
> mac is a const void *
>
> In general, casts should not be used, the indicate bad design. But the
> cast you are adding appears to be wrong, which is even worse.
>

Hello Andrew,

okay, now i understand so since mac is already of type const void *
and kfree() is already wait a parameter of type of const void * and
cast was also wrong.
I understand, I will review eth.c code better.

Regards

Ozgur

>         Andrew
>
>