Re: [PATCH v2] staging: android: ion: Replace strncpy() for stracpy()
Hi Adam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Adam-Zerella/staging-android-ion-Replace-strncpy-for-stracpy/20190912-024431
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sparc64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>> drivers/staging/android/ion/ion.c:470:3: error: implicit declaration of
>> function 'stracpy'; did you mean 'strscpy'?
>> [-Werror=implicit-function-declaration]
stracpy(hdata.name, heap->name);
^~~
strscpy
cc1: some warnings being treated as errors
vim +470 drivers/staging/android/ion/ion.c
446
447 static int ion_query_heaps(struct ion_heap_query *query)
448 {
449 struct ion_device *dev = internal_dev;
450 struct ion_heap_data __user *buffer =
u64_to_user_ptr(query->heaps);
451 int ret = -EINVAL, cnt = 0, max_cnt;
452 struct ion_heap *heap;
453 struct ion_heap_data hdata;
454
455 memset(, 0, sizeof(hdata));
456
457 down_read(>lock);
458 if (!buffer) {
459 query->cnt = dev->heap_cnt;
460 ret = 0;
461 goto out;
462 }
463
464 if (query->cnt <= 0)
465 goto out;
466
467 max_cnt = query->cnt;
468
469 plist_for_each_entry(heap, >heaps, node) {
> 470 stracpy(hdata.name, heap->name);
471 hdata.type = heap->type;
472 hdata.heap_id = heap->id;
473
474 if (copy_to_user([cnt], , sizeof(hdata))) {
475 ret = -EFAULT;
476 goto out;
477 }
478
479 cnt++;
480 if (cnt >= max_cnt)
481 break;
482 }
483
484 query->cnt = cnt;
485 ret = 0;
486 out:
487 up_read(>lock);
488 return ret;
489 }
490
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: exfat: avoid use of strcpy
No worries... We all have days like that occasionally. :P regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: exfat: avoid use of strcpy
> On 11 Sep 2019, at 21:06, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 09:53:03PM +0200, Sandro Volery wrote: >> diff --git a/drivers/staging/exfat/exfat_core.c >> b/drivers/staging/exfat/exfat_core.c >> index da8c58149c35..4336fee444ce 100644 >> --- a/drivers/staging/exfat/exfat_core.c >> +++ b/drivers/staging/exfat/exfat_core.c >> @@ -2960,18 +2960,15 @@ s32 resolve_path(struct inode *inode, char *path, >> struct chain_t *p_dir, >>struct super_block *sb = inode->i_sb; >>struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); >>struct file_id_t *fid = &(EXFAT_I(inode)->fid); >> - >> -if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) >> + > > You have added a tab here. > >> +if (strscpy(name_buf, path, sizeof(name_buf)) < 0) >>return FFS_INVALIDPATH; >> >> -strcpy(name_buf, path); >> - >>nls_cstring_to_uniname(sb, p_uniname, name_buf, ); >>if (lossy) >>return FFS_INVALIDPATH; >> >> -fid->size = i_size_read(inode); >> - >> +fid->size = i_size_read(inode); > > And you accidentally deleted some white space here. > > I use vim, so I have it configured to highlight whitespace at the end of > a line. I don't remember how it's done now but I googled it for you. > https://vim.fandom.com/wiki/Highlight_unwanted_spaces Ugh I'm so sorry I make the most stupid mistakes today.. I was so sure this time I got it right! I'll fix it tomorrow. Thanks, Sandro V ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: exfat: avoid use of strcpy
On Wed, Sep 11, 2019 at 09:53:03PM +0200, Sandro Volery wrote: > diff --git a/drivers/staging/exfat/exfat_core.c > b/drivers/staging/exfat/exfat_core.c > index da8c58149c35..4336fee444ce 100644 > --- a/drivers/staging/exfat/exfat_core.c > +++ b/drivers/staging/exfat/exfat_core.c > @@ -2960,18 +2960,15 @@ s32 resolve_path(struct inode *inode, char *path, > struct chain_t *p_dir, > struct super_block *sb = inode->i_sb; > struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); > struct file_id_t *fid = &(EXFAT_I(inode)->fid); > - > - if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) > + You have added a tab here. > + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) > return FFS_INVALIDPATH; > > - strcpy(name_buf, path); > - > nls_cstring_to_uniname(sb, p_uniname, name_buf, ); > if (lossy) > return FFS_INVALIDPATH; > > - fid->size = i_size_read(inode); > - > +fid->size = i_size_read(inode); And you accidentally deleted some white space here. I use vim, so I have it configured to highlight whitespace at the end of a line. I don't remember how it's done now but I googled it for you. https://vim.fandom.com/wiki/Highlight_unwanted_spaces regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] Staging: exfat: avoid use of strcpy
Replacing strcpy with strscpy and moving the length check to the same function. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- Took a couple attempts to finaly get this right :P v4: Replaced strlen check v3: Failed to replace check v2: Forgot to replace strlen check v1: original patch drivers/staging/exfat/exfat_core.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4336fee444ce 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2960,18 +2960,15 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, struct super_block *sb = inode->i_sb; struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info); struct file_id_t *fid = &(EXFAT_I(inode)->fid); - - if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) + + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) return FFS_INVALIDPATH; - strcpy(name_buf, path); - nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) return FFS_INVALIDPATH; - fid->size = i_size_read(inode); - +fid->size = i_size_read(inode); p_dir->dir = fid->start_clu; p_dir->size = (s32)(fid->size >> p_fs->cluster_size_bits); p_dir->flags = fid->flags; -- 2.23.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
On Wed, Sep 11, 2019 at 08:24:22PM +0300, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 08:33:00PM +0530, Rohit Sarkar wrote: > > There are a lot of usages of "snprintf" throughout the staging > > directory (315 to be exact) > > Would it be worthwhile to find ones that may cause an information leak > > and replace them with "scnprintf"? > > A lot of times it's really easy to see that the uses are safe, so > snprintf() is fine in that case. If it's not obviously safe then change > it. > > regards, > dan carpenter > Sure, thanks a ton! regards, Rohit ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
On Wed, Sep 11, 2019 at 08:33:00PM +0530, Rohit Sarkar wrote: > There are a lot of usages of "snprintf" throughout the staging > directory (315 to be exact) > Would it be worthwhile to find ones that may cause an information leak > and replace them with "scnprintf"? A lot of times it's really easy to see that the uses are safe, so snprintf() is fine in that case. If it's not obviously safe then change it. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: media: Fix alignment to match open parenthesis
CHECK: Alignment should match open parenthesis Signed-off-by: Amol Grover --- drivers/staging/media/imx/imx-media-csi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index 367e39f5b382..773b3d6964cf 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -627,8 +627,8 @@ static int csi_idmac_start(struct csi_priv *priv) } priv->nfb4eof_irq = ipu_idmac_channel_irq(priv->ipu, -priv->idmac_ch, -IPU_IRQ_NFB4EOF); + priv->idmac_ch, + IPU_IRQ_NFB4EOF); ret = devm_request_irq(priv->dev, priv->nfb4eof_irq, csi_idmac_nfb4eof_interrupt, 0, "imx-smfc-nfb4eof", priv); @@ -1472,7 +1472,7 @@ static void csi_try_fmt(struct csi_priv *priv, imx_media_enum_mbus_format(, 0, CS_SEL_ANY, false); *cc = imx_media_find_mbus_format(code, - CS_SEL_ANY, false); +CS_SEL_ANY, false); sdformat->format.code = (*cc)->codes[0]; } -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
On Wed, Sep 11, 2019 at 05:46:12PM +0300, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 07:55:43PM +0530, Rohit Sarkar wrote: > > On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote: > > > Hi Rohit, > > > > > > On 11.09.19 15:51, Rohit Sarkar wrote: > > > > When the number of bytes to be printed exceeds the limit snprintf > > > > returns the number of bytes that would have been printed (if there was > > > > no truncation). This might cause issues, hence use scnprintf which > > > > returns the actual number of bytes printed to buffer always > > > > > > > > Signed-off-by: Rohit Sarkar > > > thanks for your patch. Did you test your change on the Raspberry Pi? > > > > Hey Stefan, > > No I haven't done so as I thought this is a generic change? > > Will that be necessary? > > No. It's not required. The patch is easy to audit and clearly > harmless. > > The question would be does it actually fix a bug? I looked at it and > some of the strings are definitely a bit long. The longest one I saw > was: > " Slots: %d available (%d data), %d recyclable, %d stalls (%d data)", > 123456789 123456789 123456789 123456789 123456789 123456789 123456789 > > If you get a lot of stalls, then that looks like it could lead to a > read overflow (an information leak). Either way this does make the > code a bit easier to audit so it seems like a nice cleanup. Next time > though, I really would prefer if you put this sort analysis in your > commit message so I can just glance over it. (I'm lazy). > > Reviewed-by: Dan Carpenter > > regards, > dan carpenter Hey Dan, Thanks for reviewing this :) I will make sure to add some analysis the next time I do a clean up like this. There are a lot of usages of "snprintf" throughout the staging directory (315 to be exact) Would it be worthwhile to find ones that may cause an information leak and replace them with "scnprintf"? Thanks, Rohit ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
On Wed, Sep 11, 2019 at 07:55:43PM +0530, Rohit Sarkar wrote: > On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote: > > Hi Rohit, > > > > On 11.09.19 15:51, Rohit Sarkar wrote: > > > When the number of bytes to be printed exceeds the limit snprintf > > > returns the number of bytes that would have been printed (if there was > > > no truncation). This might cause issues, hence use scnprintf which > > > returns the actual number of bytes printed to buffer always > > > > > > Signed-off-by: Rohit Sarkar > > thanks for your patch. Did you test your change on the Raspberry Pi? > > Hey Stefan, > No I haven't done so as I thought this is a generic change? > Will that be necessary? No. It's not required. The patch is easy to audit and clearly harmless. The question would be does it actually fix a bug? I looked at it and some of the strings are definitely a bit long. The longest one I saw was: " Slots: %d available (%d data), %d recyclable, %d stalls (%d data)", 123456789 123456789 123456789 123456789 123456789 123456789 123456789 If you get a lot of stalls, then that looks like it could lead to a read overflow (an information leak). Either way this does make the code a bit easier to audit so it seems like a nice cleanup. Next time though, I really would prefer if you put this sort analysis in your commit message so I can just glance over it. (I'm lazy). Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote: > Hi Rohit, > > On 11.09.19 15:51, Rohit Sarkar wrote: > > When the number of bytes to be printed exceeds the limit snprintf > > returns the number of bytes that would have been printed (if there was > > no truncation). This might cause issues, hence use scnprintf which > > returns the actual number of bytes printed to buffer always > > > > Signed-off-by: Rohit Sarkar > thanks for your patch. Did you test your change on the Raspberry Pi? Hey Stefan, No I haven't done so as I thought this is a generic change? Will that be necessary? I am relatively new to kernel development Thanks, Rohit ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
Hi Rohit, On 11.09.19 15:51, Rohit Sarkar wrote: > When the number of bytes to be printed exceeds the limit snprintf > returns the number of bytes that would have been printed (if there was > no truncation). This might cause issues, hence use scnprintf which > returns the actual number of bytes printed to buffer always > > Signed-off-by: Rohit Sarkar thanks for your patch. Did you test your change on the Raspberry Pi? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vhciq_core: replace snprintf with scnprintf
When the number of bytes to be printed exceeds the limit snprintf
returns the number of bytes that would have been printed (if there was
no truncation). This might cause issues, hence use scnprintf which
returns the actual number of bytes printed to buffer always
Signed-off-by: Rohit Sarkar
---
.../interface/vchiq_arm/vchiq_core.c | 38 +--
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 183f5cf887e0..56a23a297fa4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3322,13 +3322,13 @@ vchiq_dump_shared_state(void *dump_context, struct
vchiq_state *state,
char buf[80];
int len;
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" %s: slots %d-%d tx_pos=%x recycle=%x",
label, shared->slot_first, shared->slot_last,
shared->tx_pos, shared->slot_queue_recycle);
vchiq_dump(dump_context, buf, len + 1);
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
"Slots claimed:");
vchiq_dump(dump_context, buf, len + 1);
@@ -3336,7 +3336,7 @@ vchiq_dump_shared_state(void *dump_context, struct
vchiq_state *state,
struct vchiq_slot_info slot_info =
*SLOT_INFO_FROM_INDEX(state, i);
if (slot_info.use_count != slot_info.release_count) {
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" %d: %d/%d", i, slot_info.use_count,
slot_info.release_count);
vchiq_dump(dump_context, buf, len + 1);
@@ -3344,7 +3344,7 @@ vchiq_dump_shared_state(void *dump_context, struct
vchiq_state *state,
}
for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
- len = snprintf(buf, sizeof(buf), "DEBUG: %s = %d(%x)",
+ len = scnprintf(buf, sizeof(buf), "DEBUG: %s = %d(%x)",
debug_names[i], shared->debug[i], shared->debug[i]);
vchiq_dump(dump_context, buf, len + 1);
}
@@ -3357,11 +3357,11 @@ vchiq_dump_state(void *dump_context, struct vchiq_state
*state)
int len;
int i;
- len = snprintf(buf, sizeof(buf), "State %d: %s", state->id,
+ len = scnprintf(buf, sizeof(buf), "State %d: %s", state->id,
conn_state_names[state->conn_state]);
vchiq_dump(dump_context, buf, len + 1);
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" tx_pos=%x(@%pK), rx_pos=%x(@%pK)",
state->local->tx_pos,
state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
@@ -3369,13 +3369,13 @@ vchiq_dump_state(void *dump_context, struct vchiq_state
*state)
state->rx_data + (state->rx_pos & VCHIQ_SLOT_MASK));
vchiq_dump(dump_context, buf, len + 1);
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" Version: %d (min %d)",
VCHIQ_VERSION, VCHIQ_VERSION_MIN);
vchiq_dump(dump_context, buf, len + 1);
if (VCHIQ_ENABLE_STATS) {
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" Stats: ctrl_tx_count=%d, ctrl_rx_count=%d, "
"error_count=%d",
state->stats.ctrl_tx_count, state->stats.ctrl_rx_count,
@@ -3383,7 +3383,7 @@ vchiq_dump_state(void *dump_context, struct vchiq_state
*state)
vchiq_dump(dump_context, buf, len + 1);
}
- len = snprintf(buf, sizeof(buf),
+ len = scnprintf(buf, sizeof(buf),
" Slots: %d available (%d data), %d recyclable, %d stalls "
"(%d data)",
((state->slot_queue_available * VCHIQ_SLOT_SIZE) -
@@ -3416,7 +3416,7 @@ vchiq_dump_service_state(void *dump_context, struct
vchiq_service *service)
char buf[80];
int len;
- len = snprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
+ len = scnprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
service->localport, srvstate_names[service->srvstate],
service->ref_count - 1); /*Don't include the lock just taken*/
@@ -3428,17 +3428,17 @@ vchiq_dump_service_state(void *dump_context, struct
vchiq_service *service)
int tx_pending, rx_pending;
if (service->remoteport != VCHIQ_PORT_FREE) {
- int len2 = snprintf(remoteport, sizeof(remoteport),
+ int len2 = scnprintf(remoteport,
Re: [PATCH v3] Staging: exfat: Avoid use of strcpy
On Wed, Sep 11, 2019 at 02:48:12PM +0200, Sandro Volery wrote: > Use strscpy instead of strcpy in exfat_core.c, and add a check > for length that will return already known FFS_INVALIDPATH. > > Suggested-by: Rasmus Villemoes > Signed-off-by: Sandro Volery > --- > v3: Fixed replacing mistake > v2: Introduced length check > v1: Original patch > drivers/staging/exfat/exfat_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/exfat/exfat_core.c > b/drivers/staging/exfat/exfat_core.c > index da8c58149c35..4c40f1352848 100644 > --- a/drivers/staging/exfat/exfat_core.c > +++ b/drivers/staging/exfat/exfat_core.c > @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, > struct chain_t *p_dir, > if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) ^^^ Get rid of this. > return FFS_INVALIDPATH; > > - strcpy(name_buf, path); > + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) > + return FFS_INVALIDPATH; regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] Staging: exfat: Avoid use of strcpy
Use strscpy instead of strcpy in exfat_core.c, and add a check for length that will return already known FFS_INVALIDPATH. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- v3: Fixed replacing mistake v2: Introduced length check v1: Original patch drivers/staging/exfat/exfat_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4c40f1352848 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) return FFS_INVALIDPATH; - strcpy(name_buf, path); + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) + return FFS_INVALIDPATH; nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) -- 2.23.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On 11/09/2019 11.16, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: >> >> >>> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: >>> >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Yes, that's obviously what the patch does. The commit log is supposed to explain _why_. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; -strcpy(dev->name, "pow%d"); +strscpy(dev->name, "pow%d", sizeof(dev->name)); >>> >>> Is there a program which is generating a warning for this code? We know >>> that "pow%d" is 6 characters and static analysis tools can understand >>> this code fine so we know it's safe. >> >> Well I was confused too but checkpatch complained about >> it so I figured I'd clean it up quick > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > fine with replacing all of these in that case. But why? It actually gives _less_ compile-time checking (gcc and all static tools know perfectly well what strcpy is and does, but knows nothing of strscpy). And using sizeof() instead of ARRAY_SIZE() means a future reader is not even sure dev->name is not just a pointer. Moreover, it's very likely also a runtime and .text pessimization, again because gcc knows what strcpy does, so it can just do a few immediate stores (e.g. 0x25776f70 for the "pow%" part) instead of emitting an actual function call. Rasmus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] staging: rtl8192u: Add or remove spaces to fix style issues
On Tue, Sep 10, 2019 at 5:00 PM Dan Carpenter wrote: > > On Sun, Sep 08, 2019 at 12:58:39PM +0530, Sumera Priyadarsini wrote: > > Hi, I am extremely sorry for a late response to this. I was caught up > > with some laptop issues. I will send a separate patch making the > > change right away. > > There is not need to apologize at all. I'm not signing your paychecks. > Take as long as you want. :P > > regards, > dan carpenter > I sent a patch. Would you please review it? regards, sumera ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] Staging: exfat: Avoid use of strcpy
On Wed, Sep 11, 2019 at 12:24:23PM +0200, Sandro Volery wrote: > > > > On 11 Sep 2019, at 12:06, Dan Carpenter wrote: > > > > On Wed, Sep 11, 2019 at 11:42:19AM +0200, Sandro Volery wrote: > >> Use strscpy instead of strcpy in exfat_core.c, and add a check > >> for length that will return already known FFS_INVALIDPATH. > >> > >> Suggested-by: Rasmus Villemoes > >> Signed-off-by: Sandro Volery > >> --- > >> v2: Implement length check and return in one > >> v1: Original Patch > >> drivers/staging/exfat/exfat_core.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/exfat/exfat_core.c > >> b/drivers/staging/exfat/exfat_core.c > >> index da8c58149c35..4c40f1352848 100644 > >> --- a/drivers/staging/exfat/exfat_core.c > >> +++ b/drivers/staging/exfat/exfat_core.c > >> @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, > >> struct chain_t *p_dir, > >>if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) > > > > Delete this as it is no longer required. > > > > Yep, saw it from Rasmus response too just now.. Dumb mistake. > Will fix this this afternoon. > Or you could send it tomorrow. There is no rush. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] Staging: exfat: Avoid use of strcpy
> On 11 Sep 2019, at 12:06, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 11:42:19AM +0200, Sandro Volery wrote: >> Use strscpy instead of strcpy in exfat_core.c, and add a check >> for length that will return already known FFS_INVALIDPATH. >> >> Suggested-by: Rasmus Villemoes >> Signed-off-by: Sandro Volery >> --- >> v2: Implement length check and return in one >> v1: Original Patch >> drivers/staging/exfat/exfat_core.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/staging/exfat/exfat_core.c >> b/drivers/staging/exfat/exfat_core.c >> index da8c58149c35..4c40f1352848 100644 >> --- a/drivers/staging/exfat/exfat_core.c >> +++ b/drivers/staging/exfat/exfat_core.c >> @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, >> struct chain_t *p_dir, >>if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) > > Delete this as it is no longer required. > Yep, saw it from Rasmus response too just now.. Dumb mistake. Will fix this this afternoon. Sandro V >>return FFS_INVALIDPATH; >> >> -strcpy(name_buf, path); >> +if (strscpy(name_buf, path, sizeof(name_buf)) < 0) >> +return FFS_INVALIDPATH; > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 11:21:44AM +0200, Sandro Volery wrote: > > On 11 Sep 2019, at 11:17, Dan Carpenter wrote: > > > > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: > >> > >> > On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > >>> > >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > strcpy was used multiple times in strcpy to write into dev->name. > I replaced them with strscpy. > > Signed-off-by: Sandro Volery > --- > drivers/staging/octeon/ethernet.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/octeon/ethernet.c > b/drivers/staging/octeon/ethernet.c > index 8889494adf1f..cf8e9a23ebf9 100644 > --- a/drivers/staging/octeon/ethernet.c > +++ b/drivers/staging/octeon/ethernet.c > @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device > *pdev) > priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > priv->port = CVMX_PIP_NUM_INPUT_PORTS; > priv->queue = -1; > -strcpy(dev->name, "pow%d"); > +strscpy(dev->name, "pow%d", sizeof(dev->name)); > >>> > >>> Is there a program which is generating a warning for this code? We know > >>> that "pow%d" is 6 characters and static analysis tools can understand > >>> this code fine so we know it's safe. > >> > >> Well I was confused too but checkpatch complained about > >> it so I figured I'd clean it up quick > > > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > > fine with replacing all of these in that case. > > Alright thanks. Can you review this? > Sure. Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] Staging: exfat: Avoid use of strcpy
On Wed, Sep 11, 2019 at 11:42:19AM +0200, Sandro Volery wrote: > Use strscpy instead of strcpy in exfat_core.c, and add a check > for length that will return already known FFS_INVALIDPATH. > > Suggested-by: Rasmus Villemoes > Signed-off-by: Sandro Volery > --- > v2: Implement length check and return in one > v1: Original Patch > drivers/staging/exfat/exfat_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/exfat/exfat_core.c > b/drivers/staging/exfat/exfat_core.c > index da8c58149c35..4c40f1352848 100644 > --- a/drivers/staging/exfat/exfat_core.c > +++ b/drivers/staging/exfat/exfat_core.c > @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, > struct chain_t *p_dir, > if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) Delete this as it is no longer required. > return FFS_INVALIDPATH; > > - strcpy(name_buf, path); > + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) > + return FFS_INVALIDPATH; regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] Staging: exfat: Avoid use of strcpy
Use strscpy instead of strcpy in exfat_core.c, and add a check for length that will return already known FFS_INVALIDPATH. Suggested-by: Rasmus Villemoes Signed-off-by: Sandro Volery --- v2: Implement length check and return in one v1: Original Patch drivers/staging/exfat/exfat_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c index da8c58149c35..4c40f1352848 100644 --- a/drivers/staging/exfat/exfat_core.c +++ b/drivers/staging/exfat/exfat_core.c @@ -2964,7 +2964,8 @@ s32 resolve_path(struct inode *inode, char *path, struct chain_t *p_dir, if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) return FFS_INVALIDPATH; - strcpy(name_buf, path); + if (strscpy(name_buf, path, sizeof(name_buf)) < 0) + return FFS_INVALIDPATH; nls_cstring_to_uniname(sb, p_uniname, name_buf, ); if (lossy) -- 2.23.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On 11 Sep 2019, at 11:17, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: >> >> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: >>> >>> On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; -strcpy(dev->name, "pow%d"); +strscpy(dev->name, "pow%d", sizeof(dev->name)); >>> >>> Is there a program which is generating a warning for this code? We know >>> that "pow%d" is 6 characters and static analysis tools can understand >>> this code fine so we know it's safe. >> >> Well I was confused too but checkpatch complained about >> it so I figured I'd clean it up quick > > Ah. It's a new checkpatch warning. I don't care in that case. I'm > fine with replacing all of these in that case. Alright thanks. Can you review this? Thanks, Sandro V ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 11:04:38AM +0200, Sandro Volery wrote: > > > > On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > > > > On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > >> strcpy was used multiple times in strcpy to write into dev->name. > >> I replaced them with strscpy. > >> > >> Signed-off-by: Sandro Volery > >> --- > >> drivers/staging/octeon/ethernet.c | 16 > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/staging/octeon/ethernet.c > >> b/drivers/staging/octeon/ethernet.c > >> index 8889494adf1f..cf8e9a23ebf9 100644 > >> --- a/drivers/staging/octeon/ethernet.c > >> +++ b/drivers/staging/octeon/ethernet.c > >> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) > >>priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > >>priv->port = CVMX_PIP_NUM_INPUT_PORTS; > >>priv->queue = -1; > >> -strcpy(dev->name, "pow%d"); > >> +strscpy(dev->name, "pow%d", sizeof(dev->name)); > > > > Is there a program which is generating a warning for this code? We know > > that "pow%d" is 6 characters and static analysis tools can understand > > this code fine so we know it's safe. > > Well I was confused too but checkpatch complained about > it so I figured I'd clean it up quick Ah. It's a new checkpatch warning. I don't care in that case. I'm fine with replacing all of these in that case. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
> On 11 Sep 2019, at 10:52, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: >> strcpy was used multiple times in strcpy to write into dev->name. >> I replaced them with strscpy. >> >> Signed-off-by: Sandro Volery >> --- >> drivers/staging/octeon/ethernet.c | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/staging/octeon/ethernet.c >> b/drivers/staging/octeon/ethernet.c >> index 8889494adf1f..cf8e9a23ebf9 100644 >> --- a/drivers/staging/octeon/ethernet.c >> +++ b/drivers/staging/octeon/ethernet.c >> @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) >>priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; >>priv->port = CVMX_PIP_NUM_INPUT_PORTS; >>priv->queue = -1; >> -strcpy(dev->name, "pow%d"); >> +strscpy(dev->name, "pow%d", sizeof(dev->name)); > > Is there a program which is generating a warning for this code? We know > that "pow%d" is 6 characters and static analysis tools can understand > this code fine so we know it's safe. Well I was confused too but checkpatch complained about it so I figured I'd clean it up quick ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: exfat: Avoid use of strcpy
On 11/09/2019 10.41, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 07:57:49AM +0200, Sandro Volery wrote: >> Replaced strcpy with strscpy in exfat_core.c. >> >> Signed-off-by: Sandro Volery >> --- >> drivers/staging/exfat/exfat_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/exfat/exfat_core.c >> b/drivers/staging/exfat/exfat_core.c >> index da8c58149c35..c71b145e8a24 100644 >> --- a/drivers/staging/exfat/exfat_core.c >> +++ b/drivers/staging/exfat/exfat_core.c >> @@ -2964,7 +2964,7 @@ s32 resolve_path(struct inode *inode, char *path, >> struct chain_t *p_dir, >> if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) >> return FFS_INVALIDPATH; >> >> -strcpy(name_buf, path); >> +strscpy(name_buf, path, sizeof(name_buf)); > > It checked strlen() earlier so we know that it can't overflow but, oh > wow, the "name_buf" is a shared buffer. wow wow wow. This seems very > racy. Yeah, and note that the callers of resolve_path do seem to take a lock, but that's not a global lock but a per-superblock (or per-something-else) one... Obviously exfat should not rely on such a single static buffer, but in the meantime, perhaps one should add a name_buf_lock (though I don't know how long name_buf is actually in use, so that needs checking). Apart from the broken/lack of locking, it would be better to combine the copying and length checking into a single check - i.e. do if (strscpy(name_buf, path, sizeof(name_buf)) < 0) return FFS_INVALIDPATH; That's both more efficient and fixes the open-coding of sizeof(name_buf) that is currently used in the if(). Rasmus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: octeon: Avoid several usecases of strcpy
On Wed, Sep 11, 2019 at 08:23:59AM +0200, Sandro Volery wrote: > strcpy was used multiple times in strcpy to write into dev->name. > I replaced them with strscpy. > > Signed-off-by: Sandro Volery > --- > drivers/staging/octeon/ethernet.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/octeon/ethernet.c > b/drivers/staging/octeon/ethernet.c > index 8889494adf1f..cf8e9a23ebf9 100644 > --- a/drivers/staging/octeon/ethernet.c > +++ b/drivers/staging/octeon/ethernet.c > @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) > priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; > priv->port = CVMX_PIP_NUM_INPUT_PORTS; > priv->queue = -1; > - strcpy(dev->name, "pow%d"); > + strscpy(dev->name, "pow%d", sizeof(dev->name)); Is there a program which is generating a warning for this code? We know that "pow%d" is 6 characters and static analysis tools can understand this code fine so we know it's safe. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: exfat: Avoid use of strcpy
On Wed, Sep 11, 2019 at 07:57:49AM +0200, Sandro Volery wrote: > Replaced strcpy with strscpy in exfat_core.c. > > Signed-off-by: Sandro Volery > --- > drivers/staging/exfat/exfat_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/exfat/exfat_core.c > b/drivers/staging/exfat/exfat_core.c > index da8c58149c35..c71b145e8a24 100644 > --- a/drivers/staging/exfat/exfat_core.c > +++ b/drivers/staging/exfat/exfat_core.c > @@ -2964,7 +2964,7 @@ s32 resolve_path(struct inode *inode, char *path, > struct chain_t *p_dir, > if (strlen(path) >= (MAX_NAME_LENGTH * MAX_CHARSET_SIZE)) > return FFS_INVALIDPATH; > > - strcpy(name_buf, path); > + strscpy(name_buf, path, sizeof(name_buf)); It checked strlen() earlier so we know that it can't overflow but, oh wow, the "name_buf" is a shared buffer. wow wow wow. This seems very racy. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: octeon: Avoid several usecases of strcpy
strcpy was used multiple times in strcpy to write into dev->name. I replaced them with strscpy. Signed-off-by: Sandro Volery --- drivers/staging/octeon/ethernet.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 8889494adf1f..cf8e9a23ebf9 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -784,7 +784,7 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->imode = CVMX_HELPER_INTERFACE_MODE_DISABLED; priv->port = CVMX_PIP_NUM_INPUT_PORTS; priv->queue = -1; - strcpy(dev->name, "pow%d"); + strscpy(dev->name, "pow%d", sizeof(dev->name)); for (qos = 0; qos < 16; qos++) skb_queue_head_init(>tx_free_list[qos]); dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead; @@ -856,39 +856,39 @@ static int cvm_oct_probe(struct platform_device *pdev) case CVMX_HELPER_INTERFACE_MODE_NPI: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "npi%d"); + strscpy(dev->name, "npi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_XAUI: dev->netdev_ops = _oct_xaui_netdev_ops; - strcpy(dev->name, "xaui%d"); + strscpy(dev->name, "xaui%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_LOOP: dev->netdev_ops = _oct_npi_netdev_ops; - strcpy(dev->name, "loop%d"); + strscpy(dev->name, "loop%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SGMII: priv->phy_mode = PHY_INTERFACE_MODE_SGMII; dev->netdev_ops = _oct_sgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_SPI: dev->netdev_ops = _oct_spi_netdev_ops; - strcpy(dev->name, "spi%d"); + strscpy(dev->name, "spi%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_GMII: priv->phy_mode = PHY_INTERFACE_MODE_GMII; dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); break; case CVMX_HELPER_INTERFACE_MODE_RGMII: dev->netdev_ops = _oct_rgmii_netdev_ops; - strcpy(dev->name, "eth%d"); + strscpy(dev->name, "eth%d", sizeof(dev->name)); cvm_set_rgmii_delay(priv, interface, port_index); break; -- 2.23.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
