| Message ID | 20231113083544.1685919-1-gal@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: Fix undefined behavior in netdev name allocation | expand |
On Mon, Nov 13, 2023 at 10:35:44AM +0200, Gal Pressman wrote: > Cited commit removed the strscpy() call and kept the snprintf() only. > > When allocating a netdev, 'res' and 'name' pointers are equal, but > 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: 9a810468126c ("net: reduce indentation of __dev_alloc_name()") Hi Gal, perhaps my eyes are deceiving me, but I wonder if this fixes the following: 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> > --- > net/core/dev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > 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; > } > > -- > 2.40.1 > >
On 13/11/2023 11:53, Simon Horman wrote: > On Mon, Nov 13, 2023 at 10:35:44AM +0200, Gal Pressman wrote: >> Cited commit removed the strscpy() call and kept the snprintf() only. >> >> When allocating a netdev, 'res' and 'name' pointers are equal, but >> 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: 9a810468126c ("net: reduce indentation of __dev_alloc_name()") > > Hi Gal, > > perhaps my eyes are deceiving me, but I wonder if this fixes the following: > > 7ad17b04dc7b ("net: trust the bitmap in __dev_alloc_name()") Thanks Simon, you're right. Should I resubmit?
On Mon, 13 Nov 2023 10:35:44 +0200 Gal Pressman wrote: > Cited commit removed the strscpy() call and kept the snprintf() only. > > When allocating a netdev, 'res' and 'name' pointers are equal, but > 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. It may be worth mentioning that it is fairly common to put the format in dev->name before device is registered, IOW this condition takes place a lot? IIUC once we cross into 3-digit IDs we may crash? With that and the right fixes tag: Reviewed-by: Jakub Kicinski <kuba@kernel.org> Thanks!
On Mon, 13 Nov 2023 23:09:02 -0500 Jakub Kicinski wrote:
> IIUC once we cross into 3-digit IDs we may crash?
Not really crash I guess just repeat some numbers until the end of
the buffer.
On 14/11/2023 6:09, Jakub Kicinski wrote: > On Mon, 13 Nov 2023 10:35:44 +0200 Gal Pressman wrote: >> Cited commit removed the strscpy() call and kept the snprintf() only. >> >> When allocating a netdev, 'res' and 'name' pointers are equal, but >> 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. > > It may be worth mentioning that it is fairly common to put the format > in dev->name before device is registered, IOW this condition takes > place a lot? I'll mention it. > IIUC once we cross into 3-digit IDs we may crash? Right, the bitmap and names get out of sync, it results in sysfs name collision call traces. > > With that and the right fixes tag: > > Reviewed-by: Jakub Kicinski <kuba@kernel.org> > > Thanks! Thanks Jakub!
On Mon, Nov 13, 2023 at 04:01:23PM +0200, Gal Pressman wrote: > On 13/11/2023 11:53, Simon Horman wrote: > > On Mon, Nov 13, 2023 at 10:35:44AM +0200, Gal Pressman wrote: > >> Cited commit removed the strscpy() call and kept the snprintf() only. > >> > >> When allocating a netdev, 'res' and 'name' pointers are equal, but > >> 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: 9a810468126c ("net: reduce indentation of __dev_alloc_name()") > > > > Hi Gal, > > > > perhaps my eyes are deceiving me, but I wonder if this fixes the following: > > > > 7ad17b04dc7b ("net: trust the bitmap in __dev_alloc_name()") > > Thanks Simon, you're right. > > Should I resubmit? I guess that it is not strictly necessary, but it might be a good idea as I imagine it makes things slightly easier for the maintainers. In any case, thanks for confirming and with this changed (somehow) this patch looks good to me. Reviewed-by: Simon Horman <horms@kernel.org>
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. When allocating a netdev, 'res' and 'name' pointers are equal, but 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: 9a810468126c ("net: reduce indentation of __dev_alloc_name()") Cc: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Vlad Buslov <vladbu@nvidia.com> Signed-off-by: Gal Pressman <gal@nvidia.com> --- net/core/dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)