| Message ID | 20210928201733.GA268467@embeddedor (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f69bf5dee7efdb7cbc0f3a0839989873e10cfa48 |
| Headers | show |
| Series | [net-next] net/mlx4: Use array_size() helper in copy_to_user() | expand |
On 9/28/2021 11:17 PM, Gustavo A. R. Silva wrote: > Use array_size() helper instead of the open-coded version in > copy_to_user(). These sorts of multiplication factors need > to be wrapped in array_size(). > > Link: https://github.com/KSPP/linux/issues/160 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/net/ethernet/mellanox/mlx4/cq.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c > index f7053a74e6a8..4d4f9cf9facb 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/cq.c > +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c > @@ -314,7 +314,8 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) > buf += PAGE_SIZE; > } > } else { > - err = copy_to_user((void __user *)buf, init_ents, entries * cqe_size) ? > + err = copy_to_user((void __user *)buf, init_ents, > + array_size(entries, cqe_size)) ? > -EFAULT : 0; > } > > Thanks for your patch. Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
On 9/29/21 3:24 AM, Tariq Toukan wrote: > > > On 9/28/2021 11:17 PM, Gustavo A. R. Silva wrote: >> Use array_size() helper instead of the open-coded version in >> copy_to_user(). These sorts of multiplication factors need >> to be wrapped in array_size(). >> >> Link: https://github.com/KSPP/linux/issues/160 >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/net/ethernet/mellanox/mlx4/cq.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c >> index f7053a74e6a8..4d4f9cf9facb 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/cq.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c >> @@ -314,7 +314,8 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) >> buf += PAGE_SIZE; >> } >> } else { >> - err = copy_to_user((void __user *)buf, init_ents, entries * cqe_size) ? >> + err = copy_to_user((void __user *)buf, init_ents, >> + array_size(entries, cqe_size)) ? >> -EFAULT : 0; >> } >> > > Thanks for your patch. > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Not sure why avoiding size_t overflows would make this code safer. init_ents contains PAGE_SIZE bytes... BTW Is @entries guaranteed to be a power of two ? This function seems to either copy one chunk ( <= PAGE_SIZE), or a number of full pages.
On 9/29/2021 8:21 PM, Eric Dumazet wrote: > > > On 9/29/21 3:24 AM, Tariq Toukan wrote: >> >> >> On 9/28/2021 11:17 PM, Gustavo A. R. Silva wrote: >>> Use array_size() helper instead of the open-coded version in >>> copy_to_user(). These sorts of multiplication factors need >>> to be wrapped in array_size(). >>> >>> Link: https://github.com/KSPP/linux/issues/160 >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>> --- >>> drivers/net/ethernet/mellanox/mlx4/cq.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c >>> index f7053a74e6a8..4d4f9cf9facb 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx4/cq.c >>> +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c >>> @@ -314,7 +314,8 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) >>> buf += PAGE_SIZE; >>> } >>> } else { >>> - err = copy_to_user((void __user *)buf, init_ents, entries * cqe_size) ? >>> + err = copy_to_user((void __user *)buf, init_ents, >>> + array_size(entries, cqe_size)) ? >>> -EFAULT : 0; >>> } >>> >> >> Thanks for your patch. >> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > > Not sure why avoiding size_t overflows would make this code safer. > init_ents contains PAGE_SIZE bytes... > > BTW > > Is @entries guaranteed to be a power of two ? Yes. > > This function seems to either copy one chunk ( <= PAGE_SIZE), > or a number of full pages. > Exactly. No remainder handling is needed, for the reason you mentioned above.
diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c index f7053a74e6a8..4d4f9cf9facb 100644 --- a/drivers/net/ethernet/mellanox/mlx4/cq.c +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c @@ -314,7 +314,8 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) buf += PAGE_SIZE; } } else { - err = copy_to_user((void __user *)buf, init_ents, entries * cqe_size) ? + err = copy_to_user((void __user *)buf, init_ents, + array_size(entries, cqe_size)) ? -EFAULT : 0; }
Use array_size() helper instead of the open-coded version in copy_to_user(). These sorts of multiplication factors need to be wrapped in array_size(). Link: https://github.com/KSPP/linux/issues/160 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/net/ethernet/mellanox/mlx4/cq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)