| Message ID | 20231114075618.1698547-1-gal@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 674e318089468ece99aef4796eaef7add57f36b2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: Fix undefined behavior in netdev name allocation | expand |
On Tue, Nov 14, 2023 at 09:56:18AM +0200, Gal Pressman wrote: > Cited commit removed the strscpy() call and kept the snprintf() only. > > It is common to use 'dev->name' as the format string before a netdev is > registered, this results in 'res' and 'name' pointers being equal. > According to POSIX, if copying takes place between objects that overlap > as a result of a call to sprintf() or snprintf(), the results are > undefined. > > Add back the strscpy() and use 'buf' as an intermediate buffer. > > Fixes: 7ad17b04dc7b ("net: trust the bitmap in __dev_alloc_name()") > Cc: Jakub Kicinski <kuba@kernel.org> > Reviewed-by: Vlad Buslov <vladbu@nvidia.com> > Signed-off-by: Gal Pressman <gal@nvidia.com> > Reviewed-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Simon Horman <horms@kernel.org>
Tue, Nov 14, 2023 at 08:56:18AM CET, gal@nvidia.com wrote: >Cited commit removed the strscpy() call and kept the snprintf() only. > >It is common to use 'dev->name' as the format string before a netdev is >registered, this results in 'res' and 'name' pointers being equal. >According to POSIX, if copying takes place between objects that overlap >as a result of a call to sprintf() or snprintf(), the results are >undefined. > >Add back the strscpy() and use 'buf' as an intermediate buffer. > >Fixes: 7ad17b04dc7b ("net: trust the bitmap in __dev_alloc_name()") >Cc: Jakub Kicinski <kuba@kernel.org> >Reviewed-by: Vlad Buslov <vladbu@nvidia.com> >Signed-off-by: Gal Pressman <gal@nvidia.com> >Reviewed-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Tue, 14 Nov 2023 09:56:18 +0200 you wrote: > Cited commit removed the strscpy() call and kept the snprintf() only. > > It is common to use 'dev->name' as the format string before a netdev is > registered, this results in 'res' and 'name' pointers being equal. > According to POSIX, if copying takes place between objects that overlap > as a result of a call to sprintf() or snprintf(), the results are > undefined. > > [...] Here is the summary with links: - [net,v2] net: Fix undefined behavior in netdev name allocation https://git.kernel.org/netdev/net/c/674e31808946 You are awesome, thank you!
diff --git a/net/core/dev.c b/net/core/dev.c index 0d548431f3fa..af53f6d838ce 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1119,7 +1119,9 @@ static int __dev_alloc_name(struct net *net, const char *name, char *res) if (i == max_netdevices) return -ENFILE; - snprintf(res, IFNAMSIZ, name, i); + /* 'res' and 'name' could overlap, use 'buf' as an intermediate buffer */ + strscpy(buf, name, IFNAMSIZ); + snprintf(res, IFNAMSIZ, buf, i); return i; }
Cited commit removed the strscpy() call and kept the snprintf() only. It is common to use 'dev->name' as the format string before a netdev is registered, this results in 'res' and 'name' pointers being equal. According to POSIX, if copying takes place between objects that overlap as a result of a call to sprintf() or snprintf(), the results are undefined. Add back the strscpy() and use 'buf' as an intermediate buffer. Fixes: 7ad17b04dc7b ("net: trust the bitmap in __dev_alloc_name()") Cc: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Vlad Buslov <vladbu@nvidia.com> Signed-off-by: Gal Pressman <gal@nvidia.com> Reviewed-by: Jakub Kicinski <kuba@kernel.org> --- Changelog - v1->v2: https://lore.kernel.org/all/20231113083544.1685919-1-gal@nvidia.com/ * Mention that dev->name is usually used as the format string in the commit message (Jakub). * Put the right commit in the Fixes tag (Simon). --- net/core/dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)