| Message ID | 7c1f950b-3a7d-4252-82a6-876e53078ef7@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f72207a5c0dbaaf6921cf9a6c0d2fd0bc249ea78 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() | expand |
On Tue, Jul 11, 2023 at 2:22 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The simple_write_to_buffer() function is designed to handle partial > writes. It returns negatives on error, otherwise it returns the number > of bytes that were able to be copied. This code doesn't check the > return properly. We only know that the first byte is written, the rest > of the buffer might be uninitialized. > > There is no need to use the simple_write_to_buffer() function. > Partial writes are prohibited by the "if (*ppos != 0)" check at the > start of the function. Just use memdup_user() and copy the whole > buffer. > > Fixes: d3cbb907ae57 ("netdevsim: add ACL trap reporting cookie as a metadata") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/net/netdevsim/dev.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index 6045bece2654..b4d3b9cde8bd 100644 > --- a/drivers/net/netdevsim/dev.c > +++ b/drivers/net/netdevsim/dev.c > @@ -184,13 +184,10 @@ static ssize_t nsim_dev_trap_fa_cookie_write(struct file *file, > cookie_len = (count - 1) / 2; > if ((count - 1) % 2) > return -EINVAL; > - buf = kmalloc(count, GFP_KERNEL | __GFP_NOWARN); > - if (!buf) > - return -ENOMEM; > > - ret = simple_write_to_buffer(buf, count, ppos, data, count); > - if (ret < 0) > - goto free_buf; > + buf = memdup_user(data, count); Looks good to me except that now memory comes from GFP_USER. Within limits it still looks all fine to me. Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > + if (IS_ERR(buf)) > + return PTR_ERR(buf); > > fa_cookie = kmalloc(sizeof(*fa_cookie) + cookie_len, > GFP_KERNEL | __GFP_NOWARN); > -- > 2.39.2 > >
On Tue, Jul 11, 2023 at 11:52:26AM +0300, Dan Carpenter wrote: > The simple_write_to_buffer() function is designed to handle partial > writes. It returns negatives on error, otherwise it returns the number > of bytes that were able to be copied. This code doesn't check the > return properly. We only know that the first byte is written, the rest > of the buffer might be uninitialized. > > There is no need to use the simple_write_to_buffer() function. > Partial writes are prohibited by the "if (*ppos != 0)" check at the > start of the function. Just use memdup_user() and copy the whole > buffer. > > Fixes: d3cbb907ae57 ("netdevsim: add ACL trap reporting cookie as a metadata") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
On Tue, 11 Jul 2023 11:52:26 +0300 Dan Carpenter wrote:
> Subject: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
We usually reserve the "devlink: " prefix for net/devlink/ changes
rather than driver changes, so I adjust the subject when applying.
Applied, thanks!
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 11 Jul 2023 11:52:26 +0300 you wrote: > The simple_write_to_buffer() function is designed to handle partial > writes. It returns negatives on error, otherwise it returns the number > of bytes that were able to be copied. This code doesn't check the > return properly. We only know that the first byte is written, the rest > of the buffer might be uninitialized. > > There is no need to use the simple_write_to_buffer() function. > Partial writes are prohibited by the "if (*ppos != 0)" check at the > start of the function. Just use memdup_user() and copy the whole > buffer. > > [...] Here is the summary with links: - [net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() https://git.kernel.org/netdev/net/c/f72207a5c0db You are awesome, thank you!
On Wed, Jul 12, 2023 at 12:48:06PM -0700, Jakub Kicinski wrote: > On Tue, 11 Jul 2023 11:52:26 +0300 Dan Carpenter wrote: > > Subject: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() > > We usually reserve the "devlink: " prefix for net/devlink/ changes > rather than driver changes, so I adjust the subject when applying. Thanks! I should have seen that, sorry. regards, dan carpenter
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 6045bece2654..b4d3b9cde8bd 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -184,13 +184,10 @@ static ssize_t nsim_dev_trap_fa_cookie_write(struct file *file, cookie_len = (count - 1) / 2; if ((count - 1) % 2) return -EINVAL; - buf = kmalloc(count, GFP_KERNEL | __GFP_NOWARN); - if (!buf) - return -ENOMEM; - ret = simple_write_to_buffer(buf, count, ppos, data, count); - if (ret < 0) - goto free_buf; + buf = memdup_user(data, count); + if (IS_ERR(buf)) + return PTR_ERR(buf); fa_cookie = kmalloc(sizeof(*fa_cookie) + cookie_len, GFP_KERNEL | __GFP_NOWARN);
The simple_write_to_buffer() function is designed to handle partial writes. It returns negatives on error, otherwise it returns the number of bytes that were able to be copied. This code doesn't check the return properly. We only know that the first byte is written, the rest of the buffer might be uninitialized. There is no need to use the simple_write_to_buffer() function. Partial writes are prohibited by the "if (*ppos != 0)" check at the start of the function. Just use memdup_user() and copy the whole buffer. Fixes: d3cbb907ae57 ("netdevsim: add ACL trap reporting cookie as a metadata") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/net/netdevsim/dev.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)