[PATCH v1 1/1] mac_pton: Don't access memory over expected length

[PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andy Shevchenko]

The strlen() may go too far when estimating the length of
the given string. In some cases it may go over the boundary
and crash the system which is the case according to the commit
13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").

Rectify this by switching to strnlen() for the expected
maximum length of the string.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/net_utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/net_utils.c b/lib/net_utils.c
index af525353395d..c17201df3d08 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -6,10 +6,11 @@
 
 bool mac_pton(const char *s, u8 *mac)
 {
+	size_t maxlen = 3 * ETH_ALEN - 1;
 	int i;
 
 	/* XX:XX:XX:XX:XX:XX */
-	if (strlen(s) < 3 * ETH_ALEN - 1)
+	if (strnlen(s, maxlen) < maxlen)
 		return false;
 
 	/* Don't dirty result unless string is valid MAC. */
-- 
2.35.1

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Jakub Kicinski]

On Wed,  5 Oct 2022 19:43:01 +0300 Andy Shevchenko wrote:
> The strlen() may go too far when estimating the length of
> the given string. In some cases it may go over the boundary
> and crash the system which is the case according to the commit
> 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> 
> Rectify this by switching to strnlen() for the expected
> maximum length of the string.

# Form letter - net-next is closed

We have already sent the networking pull request for 6.1
and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after 6.1-rc1 is cut.

RFC patches sent for review only are obviously welcome at any time.

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andrew Lunn]

On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> The strlen() may go too far when estimating the length of
> the given string. In some cases it may go over the boundary
> and crash the system which is the case according to the commit
> 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> 
> Rectify this by switching to strnlen() for the expected
> maximum length of the string.

This seems like something which should have a fixes: tag, and be
against net, not net-next.

	Andrew

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 10:19:49PM +0200, Andrew Lunn wrote:
> On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> > The strlen() may go too far when estimating the length of
> > the given string. In some cases it may go over the boundary
> > and crash the system which is the case according to the commit
> > 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> > 
> > Rectify this by switching to strnlen() for the expected
> > maximum length of the string.
> 
> This seems like something which should have a fixes: tag, and be
> against net, not net-next.

I can (re-)send it that way. Just need a consensus by net maintainers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andrew Lunn]

On Mon, Oct 10, 2022 at 11:29:37PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 10, 2022 at 10:19:49PM +0200, Andrew Lunn wrote:
> > On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> > > The strlen() may go too far when estimating the length of
> > > the given string. In some cases it may go over the boundary
> > > and crash the system which is the case according to the commit
> > > 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> > > 
> > > Rectify this by switching to strnlen() for the expected
> > > maximum length of the string.
> > 
> > This seems like something which should have a fixes: tag, and be
> > against net, not net-next.
> 
> I can (re-)send it that way. Just need a consensus by net maintainers.

I would probably do:

	if (strnlen(s, maxlen) != maxlen)
 		return false;

I doubt anybody is removing leading zeros in MAC addresses.

  Andrew

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Jakub Kicinski]

On Mon, 10 Oct 2022 22:19:49 +0200 Andrew Lunn wrote:
> On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> > The strlen() may go too far when estimating the length of
> > the given string. In some cases it may go over the boundary
> > and crash the system which is the case according to the commit
> > 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> > 
> > Rectify this by switching to strnlen() for the expected
> > maximum length of the string.  
> 
> This seems like something which should have a fixes: tag, and be
> against net, not net-next.

Quoting DaveM's revert mentioned in the commit message:

    First of all, the orion5x buffer is not NULL terminated.  mac_pton()
    has no business operating on non-NULL terminated buffers because
    only the caller can know that this is valid and in what manner it
    is ok to parse this NULL'less buffer.
    
    Second of all, orion5x operates on an __iomem pointer, which cannot
    be dereferenced using normal C pointer operations.  Accesses to
    such areas much be performed with the proper iomem accessors.

So AFAICT only null-terminated strings are expected here, this patch
does not fix any known issue. No need to put it in net (if it's needed
at all).

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 05:38:09PM -0700, Jakub Kicinski wrote:
> On Mon, 10 Oct 2022 22:19:49 +0200 Andrew Lunn wrote:
> > On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> > > The strlen() may go too far when estimating the length of
> > > the given string. In some cases it may go over the boundary
> > > and crash the system which is the case according to the commit
> > > 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> > > 
> > > Rectify this by switching to strnlen() for the expected
> > > maximum length of the string.  
> > 
> > This seems like something which should have a fixes: tag, and be
> > against net, not net-next.
> 
> Quoting DaveM's revert mentioned in the commit message:
> 
>     First of all, the orion5x buffer is not NULL terminated.  mac_pton()
>     has no business operating on non-NULL terminated buffers because
>     only the caller can know that this is valid and in what manner it
>     is ok to parse this NULL'less buffer.
>     
>     Second of all, orion5x operates on an __iomem pointer, which cannot
>     be dereferenced using normal C pointer operations.  Accesses to
>     such areas much be performed with the proper iomem accessors.
> 
> So AFAICT only null-terminated strings are expected here, this patch
> does not fix any known issue. No need to put it in net (if it's needed
> at all).

So, what is the decision with this patch? Should I resend that with net-next
in the subject?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 10:45:49PM +0200, Andrew Lunn wrote:
> On Mon, Oct 10, 2022 at 11:29:37PM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 10, 2022 at 10:19:49PM +0200, Andrew Lunn wrote:
> > > On Wed, Oct 05, 2022 at 07:43:01PM +0300, Andy Shevchenko wrote:
> > > > The strlen() may go too far when estimating the length of
> > > > the given string. In some cases it may go over the boundary
> > > > and crash the system which is the case according to the commit
> > > > 13a55372b64e ("ARM: orion5x: Revert commit 4904dbda41c8.").
> > > > 
> > > > Rectify this by switching to strnlen() for the expected
> > > > maximum length of the string.
> > > 
> > > This seems like something which should have a fixes: tag, and be
> > > against net, not net-next.
> > 
> > I can (re-)send it that way. Just need a consensus by net maintainers.
> 
> I would probably do:
> 
> 	if (strnlen(s, maxlen) != maxlen)
>  		return false;
> 
> I doubt anybody is removing leading zeros in MAC addresses.

I'm not sure what this change gives us. < maxlen is more readable to understand
that we refuse anything that less than maxlen, with your change it's easy to
misinterpret it (by missing 'n' in the non-standard call) as "it must equal to
maxlen" which is obviously not true.

That said, I leave it as is.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mac_pton: Don't access memory over expected length

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 03:51:33PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 10, 2022 at 05:38:09PM -0700, Jakub Kicinski wrote:

...

> So, what is the decision with this patch? Should I resend that with net-next
> in the subject?

Found the answer in the other mail in this thread :-)

-- 
With Best Regards,
Andy Shevchenko