Re: [PATCH RESEND 04/10] crypto: hisilicon/zip - replace 'sprintf' with 'scnprintf'

2020-09-04 Thread shenyang (M) 




On 2020/9/4 15:40, Herbert Xu wrote:

On Wed, Aug 26, 2020 at 04:56:40PM +0800, shenyang (M) wrote:



@@ -514,13 +514,16 @@ static int hisi_zip_core_debug_init(struct hisi_qm *qm)
struct debugfs_regset32 *regset;
struct dentry *tmp_d;
char buf[HZIP_BUF_SIZE];
-   int i;
+   int i, ret;

for (i = 0; i < HZIP_CORE_NUM; i++) {
if (i < HZIP_COMP_CORE_NUM)
-   sprintf(buf, "comp_core%d", i);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "comp_core%d", i);
else
-   sprintf(buf, "decomp_core%d", i - HZIP_COMP_CORE_NUM);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "decomp_core%d",
+   i - HZIP_COMP_CORE_NUM);
+   if (!ret)
+   return -ENOMEM;


and that is just so wrong - did you even try to test
the 'buffer too small' code path?


Do you means the check is unnecessary?


No he's saying that your patch does the wrong thing when the string
is truncated.

Also ENOMEM is a strange error for that case.

Cheers,



Here 'HZIP_BUF_SIZE' is 22, so the buf is enough for the string.

The check for the return is really unnecessary, I will remove it in the
next version.

Thanks,
Yang

Re: [PATCH RESEND 04/10] crypto: hisilicon/zip - replace 'sprintf' with 'scnprintf'

2020-09-04 Thread Herbert Xu 
On Wed, Aug 26, 2020 at 04:56:40PM +0800, shenyang (M) wrote:
>
> > > @@ -514,13 +514,16 @@ static int hisi_zip_core_debug_init(struct hisi_qm 
> > > *qm)
> > >   struct debugfs_regset32 *regset;
> > >   struct dentry *tmp_d;
> > >   char buf[HZIP_BUF_SIZE];
> > > - int i;
> > > + int i, ret;
> > > 
> > >   for (i = 0; i < HZIP_CORE_NUM; i++) {
> > >   if (i < HZIP_COMP_CORE_NUM)
> > > - sprintf(buf, "comp_core%d", i);
> > > + ret = scnprintf(buf, HZIP_BUF_SIZE, "comp_core%d", i);
> > >   else
> > > - sprintf(buf, "decomp_core%d", i - HZIP_COMP_CORE_NUM);
> > > + ret = scnprintf(buf, HZIP_BUF_SIZE, "decomp_core%d",
> > > + i - HZIP_COMP_CORE_NUM);
> > > + if (!ret)
> > > + return -ENOMEM;
> > 
> > and that is just so wrong - did you even try to test
> > the 'buffer too small' code path?
> 
> Do you means the check is unnecessary?

No he's saying that your patch does the wrong thing when the string
is truncated.

Also ENOMEM is a strange error for that case.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH RESEND 04/10] crypto: hisilicon/zip - replace 'sprintf' with 'scnprintf'

2020-08-26 Thread shenyang (M) 




On 2020/8/24 16:29, David Laight wrote:

From: Yang Shen

Sent: 24 August 2020 04:12

Replace 'sprintf' with 'scnprintf' to avoid overrun.

Signed-off-by: Yang Shen 
Reviewed-by: Zhou Wang 
---
 drivers/crypto/hisilicon/zip/zip_main.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/hisilicon/zip/zip_main.c 
b/drivers/crypto/hisilicon/zip/zip_main.c
index df1a16f..1883d1b 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -428,7 +428,7 @@ static ssize_t hisi_zip_ctrl_debug_read(struct file *filp, 
char __user *buf,
return -EINVAL;
}
spin_unlock_irq(>lock);
-   ret = sprintf(tbuf, "%u\n", val);
+   ret = scnprintf(tbuf, HZIP_BUF_SIZE, "%u\n", val);


Should that be sizeof (tbuf).


return simple_read_from_buffer(buf, count, pos, tbuf, ret);
 }

@@ -514,13 +514,16 @@ static int hisi_zip_core_debug_init(struct hisi_qm *qm)
struct debugfs_regset32 *regset;
struct dentry *tmp_d;
char buf[HZIP_BUF_SIZE];
-   int i;
+   int i, ret;

for (i = 0; i < HZIP_CORE_NUM; i++) {
if (i < HZIP_COMP_CORE_NUM)
-   sprintf(buf, "comp_core%d", i);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "comp_core%d", i);
else
-   sprintf(buf, "decomp_core%d", i - HZIP_COMP_CORE_NUM);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "decomp_core%d",
+   i - HZIP_COMP_CORE_NUM);
+   if (!ret)
+   return -ENOMEM;


and that is just so wrong - did you even try to test
the 'buffer too small' code path?

David



Do you means the check is unnecessary?

Yang

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)


.

RE: [PATCH RESEND 04/10] crypto: hisilicon/zip - replace 'sprintf' with 'scnprintf'

2020-08-24 Thread David Laight 
From: Yang Shen
> Sent: 24 August 2020 04:12
> 
> Replace 'sprintf' with 'scnprintf' to avoid overrun.
> 
> Signed-off-by: Yang Shen 
> Reviewed-by: Zhou Wang 
> ---
>  drivers/crypto/hisilicon/zip/zip_main.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/hisilicon/zip/zip_main.c 
> b/drivers/crypto/hisilicon/zip/zip_main.c
> index df1a16f..1883d1b 100644
> --- a/drivers/crypto/hisilicon/zip/zip_main.c
> +++ b/drivers/crypto/hisilicon/zip/zip_main.c
> @@ -428,7 +428,7 @@ static ssize_t hisi_zip_ctrl_debug_read(struct file 
> *filp, char __user *buf,
>   return -EINVAL;
>   }
>   spin_unlock_irq(>lock);
> - ret = sprintf(tbuf, "%u\n", val);
> + ret = scnprintf(tbuf, HZIP_BUF_SIZE, "%u\n", val);

Should that be sizeof (tbuf).

>   return simple_read_from_buffer(buf, count, pos, tbuf, ret);
>  }
> 
> @@ -514,13 +514,16 @@ static int hisi_zip_core_debug_init(struct hisi_qm *qm)
>   struct debugfs_regset32 *regset;
>   struct dentry *tmp_d;
>   char buf[HZIP_BUF_SIZE];
> - int i;
> + int i, ret;
> 
>   for (i = 0; i < HZIP_CORE_NUM; i++) {
>   if (i < HZIP_COMP_CORE_NUM)
> - sprintf(buf, "comp_core%d", i);
> + ret = scnprintf(buf, HZIP_BUF_SIZE, "comp_core%d", i);
>   else
> - sprintf(buf, "decomp_core%d", i - HZIP_COMP_CORE_NUM);
> + ret = scnprintf(buf, HZIP_BUF_SIZE, "decomp_core%d",
> + i - HZIP_COMP_CORE_NUM);
> + if (!ret)
> + return -ENOMEM;

and that is just so wrong - did you even try to test
the 'buffer too small' code path?

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

[PATCH RESEND 04/10] crypto: hisilicon/zip - replace 'sprintf' with 'scnprintf'

2020-08-23 Thread Yang Shen 
Replace 'sprintf' with 'scnprintf' to avoid overrun.

Signed-off-by: Yang Shen 
Reviewed-by: Zhou Wang 
---
 drivers/crypto/hisilicon/zip/zip_main.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/hisilicon/zip/zip_main.c 
b/drivers/crypto/hisilicon/zip/zip_main.c
index df1a16f..1883d1b 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -428,7 +428,7 @@ static ssize_t hisi_zip_ctrl_debug_read(struct file *filp, 
char __user *buf,
return -EINVAL;
}
spin_unlock_irq(>lock);
-   ret = sprintf(tbuf, "%u\n", val);
+   ret = scnprintf(tbuf, HZIP_BUF_SIZE, "%u\n", val);
return simple_read_from_buffer(buf, count, pos, tbuf, ret);
 }
 
@@ -514,13 +514,16 @@ static int hisi_zip_core_debug_init(struct hisi_qm *qm)
struct debugfs_regset32 *regset;
struct dentry *tmp_d;
char buf[HZIP_BUF_SIZE];
-   int i;
+   int i, ret;
 
for (i = 0; i < HZIP_CORE_NUM; i++) {
if (i < HZIP_COMP_CORE_NUM)
-   sprintf(buf, "comp_core%d", i);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "comp_core%d", i);
else
-   sprintf(buf, "decomp_core%d", i - HZIP_COMP_CORE_NUM);
+   ret = scnprintf(buf, HZIP_BUF_SIZE, "decomp_core%d",
+   i - HZIP_COMP_CORE_NUM);
+   if (!ret)
+   return -ENOMEM;
 
regset = devm_kzalloc(dev, sizeof(*regset), GFP_KERNEL);
if (!regset)
-- 
2.7.4