| Message ID | 20221005164301.14381-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,1/1] mac_pton: Don't access memory over expected length | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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
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.
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
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).
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?
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.
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 :-)
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. */
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(-)