* [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
@ 2023-07-11 8:52 Dan Carpenter
2023-07-12 5:29 ` Pavan Chebbi
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-07-11 8:52 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, David S. Miller, Eric Dumazet, Paolo Abeni,
Ido Schimmel, netdev, kernel-janitors
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);
+ if (IS_ERR(buf))
+ return PTR_ERR(buf);
fa_cookie = kmalloc(sizeof(*fa_cookie) + cookie_len,
GFP_KERNEL | __GFP_NOWARN);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
2023-07-11 8:52 [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() Dan Carpenter
@ 2023-07-12 5:29 ` Pavan Chebbi
2023-07-12 8:36 ` Ido Schimmel
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Pavan Chebbi @ 2023-07-12 5:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Pirko, Jakub Kicinski, David S. Miller, Eric Dumazet,
Paolo Abeni, Ido Schimmel, netdev, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]
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
>
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
2023-07-11 8:52 [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() Dan Carpenter
2023-07-12 5:29 ` Pavan Chebbi
@ 2023-07-12 8:36 ` Ido Schimmel
2023-07-12 19:48 ` Jakub Kicinski
2023-07-12 21:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Ido Schimmel @ 2023-07-12 8:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Pirko, Jakub Kicinski, David S. Miller, Eric Dumazet,
Paolo Abeni, Ido Schimmel, netdev, kernel-janitors
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
2023-07-11 8:52 [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() Dan Carpenter
2023-07-12 5:29 ` Pavan Chebbi
2023-07-12 8:36 ` Ido Schimmel
@ 2023-07-12 19:48 ` Jakub Kicinski
2023-07-13 5:56 ` Dan Carpenter
2023-07-12 21:00 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-07-12 19:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Ido Schimmel, netdev, kernel-janitors
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
2023-07-11 8:52 [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() Dan Carpenter
` (2 preceding siblings ...)
2023-07-12 19:48 ` Jakub Kicinski
@ 2023-07-12 21:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-07-12 21:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: jiri, kuba, davem, edumazet, pabeni, idosch, netdev,
kernel-janitors
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write()
2023-07-12 19:48 ` Jakub Kicinski
@ 2023-07-13 5:56 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-07-13 5:56 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Ido Schimmel, netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-13 5:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-11 8:52 [PATCH net] devlink: uninitialized data in nsim_dev_trap_fa_cookie_write() Dan Carpenter
2023-07-12 5:29 ` Pavan Chebbi
2023-07-12 8:36 ` Ido Schimmel
2023-07-12 19:48 ` Jakub Kicinski
2023-07-13 5:56 ` Dan Carpenter
2023-07-12 21:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).