| Message ID | 20210222151231.22572-15-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Manual replacement of all strlcpy in favor of strscpy | expand |
On 22.02.21 16:12, Romain Perier wrote: > The strlcpy() reads the entire source buffer first, it is dangerous if > the source buffer lenght is unbounded or possibility non NULL-terminated. > It can lead to linear read overflows, crashes, etc... > > As recommended in the deprecated interfaces [1], it should be replaced > by strscpy. > > This commit replaces all calls to strlcpy that handle the return values > by the corresponding strscpy calls with new handling of the return > values (as it is quite different between the two functions). > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > drivers/target/target_core_configfs.c | 33 +++++++++------------------------ > 1 file changed, 9 insertions(+), 24 deletions(-) > > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c > index f04352285155..676215cd8847 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1325,16 +1325,11 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item, > /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ > unsigned char buf[INQUIRY_VENDOR_LEN + 2]; > char *stripped = NULL; > - size_t len; > + ssize_t len; > ssize_t ret; > > - len = strlcpy(buf, page, sizeof(buf)); > - if (len < sizeof(buf)) { > - /* Strip any newline added from userspace. */ > - stripped = strstrip(buf); > - len = strlen(stripped); > - } > - if (len > INQUIRY_VENDOR_LEN) { > + len = strscpy(buf, page, sizeof(buf)); > + if (len == -E2BIG) { > pr_err("Emulated T10 Vendor Identification exceeds" > " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN) > "\n"); > @@ -1381,16 +1376,11 @@ static ssize_t target_wwn_product_id_store(struct config_item *item, > /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ > unsigned char buf[INQUIRY_MODEL_LEN + 2]; > char *stripped = NULL; > - size_t len; > + ssize_t len; > ssize_t ret; > > - len = strlcpy(buf, page, sizeof(buf)); > - if (len < sizeof(buf)) { > - /* Strip any newline added from userspace. */ > - stripped = strstrip(buf); > - len = strlen(stripped); > - } > - if (len > INQUIRY_MODEL_LEN) { > + len = strscpy(buf, page, sizeof(buf)); > + if (len == -E2BIG) { > pr_err("Emulated T10 Vendor exceeds INQUIRY_MODEL_LEN: " > __stringify(INQUIRY_MODEL_LEN) > "\n"); > @@ -1437,16 +1427,11 @@ static ssize_t target_wwn_revision_store(struct config_item *item, > /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ > unsigned char buf[INQUIRY_REVISION_LEN + 2]; > char *stripped = NULL; > - size_t len; > + ssize_t len; > ssize_t ret; > > - len = strlcpy(buf, page, sizeof(buf)); > - if (len < sizeof(buf)) { > - /* Strip any newline added from userspace. */ > - stripped = strstrip(buf); > - len = strlen(stripped); > - } > - if (len > INQUIRY_REVISION_LEN) { > + len = strscpy(buf, page, sizeof(buf)); > + if (len == -E2BIG) { > pr_err("Emulated T10 Revision exceeds INQUIRY_REVISION_LEN: " > __stringify(INQUIRY_REVISION_LEN) > "\n"); > AFAICS, you are not only replacing strlcpy with strscpy, but also remove stripping of possible trailing '\n', and remove the necessary length check of the remaining string. -Bodo
Hi Romain,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on s390/features]
[also build test WARNING on cryptodev/master crypto/master driver-core/driver-core-testing linus/master v5.11 next-20210222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Romain-Perier/Manual-replacement-of-all-strlcpy-in-favor-of-strscpy/20210222-232701
base: https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: x86_64-randconfig-s022-20210222 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-229-g60c1f270-dirty
# https://github.com/0day-ci/linux/commit/8b76bc6bcdb6b3d4847d4c6298b53759acc0849a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Romain-Perier/Manual-replacement-of-all-strlcpy-in-favor-of-strscpy/20210222-232701
git checkout 8b76bc6bcdb6b3d4847d4c6298b53759acc0849a
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/target/target_core_configfs.c: In function 'target_check_inquiry_data.constprop':
>> drivers/target/target_core_configfs.c:1293:8: warning: argument 1 null where non-null expected [-Wnonnull]
1293 | len = strlen(buf);
| ^~~~~~~~~~~
In file included from include/linux/bitmap.h:9,
from include/linux/cpumask.h:12,
from arch/x86/include/asm/cpumask.h:5,
from arch/x86/include/asm/msr.h:11,
from arch/x86/include/asm/processor.h:22,
from arch/x86/include/asm/timex.h:5,
from include/linux/timex.h:65,
from include/linux/time32.h:13,
from include/linux/time.h:60,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/target/target_core_configfs.c:15:
include/linux/string.h:89:24: note: in a call to function 'strlen' declared here
89 | extern __kernel_size_t strlen(const char *);
| ^~~~~~
vim +1293 drivers/target/target_core_configfs.c
c66ac9db8d4ad9 Nicholas Bellinger 2010-12-17 1287
0322913cab79e4 Alan Adamson 2019-03-01 1288 static ssize_t target_check_inquiry_data(char *buf)
0322913cab79e4 Alan Adamson 2019-03-01 1289 {
0322913cab79e4 Alan Adamson 2019-03-01 1290 size_t len;
0322913cab79e4 Alan Adamson 2019-03-01 1291 int i;
0322913cab79e4 Alan Adamson 2019-03-01 1292
0322913cab79e4 Alan Adamson 2019-03-01 @1293 len = strlen(buf);
0322913cab79e4 Alan Adamson 2019-03-01 1294
0322913cab79e4 Alan Adamson 2019-03-01 1295 /*
0322913cab79e4 Alan Adamson 2019-03-01 1296 * SPC 4.3.1:
0322913cab79e4 Alan Adamson 2019-03-01 1297 * ASCII data fields shall contain only ASCII printable characters
0322913cab79e4 Alan Adamson 2019-03-01 1298 * (i.e., code values 20h to 7Eh) and may be terminated with one or
0322913cab79e4 Alan Adamson 2019-03-01 1299 * more ASCII null (00h) characters.
0322913cab79e4 Alan Adamson 2019-03-01 1300 */
0322913cab79e4 Alan Adamson 2019-03-01 1301 for (i = 0; i < len; i++) {
0322913cab79e4 Alan Adamson 2019-03-01 1302 if (buf[i] < 0x20 || buf[i] > 0x7E) {
0322913cab79e4 Alan Adamson 2019-03-01 1303 pr_err("Emulated T10 Inquiry Data contains non-ASCII-printable characters\n");
0322913cab79e4 Alan Adamson 2019-03-01 1304 return -EINVAL;
0322913cab79e4 Alan Adamson 2019-03-01 1305 }
0322913cab79e4 Alan Adamson 2019-03-01 1306 }
0322913cab79e4 Alan Adamson 2019-03-01 1307
0322913cab79e4 Alan Adamson 2019-03-01 1308 return len;
0322913cab79e4 Alan Adamson 2019-03-01 1309 }
0322913cab79e4 Alan Adamson 2019-03-01 1310
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c index f04352285155..676215cd8847 100644 --- a/drivers/target/target_core_configfs.c +++ b/drivers/target/target_core_configfs.c @@ -1325,16 +1325,11 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item, /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ unsigned char buf[INQUIRY_VENDOR_LEN + 2]; char *stripped = NULL; - size_t len; + ssize_t len; ssize_t ret; - len = strlcpy(buf, page, sizeof(buf)); - if (len < sizeof(buf)) { - /* Strip any newline added from userspace. */ - stripped = strstrip(buf); - len = strlen(stripped); - } - if (len > INQUIRY_VENDOR_LEN) { + len = strscpy(buf, page, sizeof(buf)); + if (len == -E2BIG) { pr_err("Emulated T10 Vendor Identification exceeds" " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN) "\n"); @@ -1381,16 +1376,11 @@ static ssize_t target_wwn_product_id_store(struct config_item *item, /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ unsigned char buf[INQUIRY_MODEL_LEN + 2]; char *stripped = NULL; - size_t len; + ssize_t len; ssize_t ret; - len = strlcpy(buf, page, sizeof(buf)); - if (len < sizeof(buf)) { - /* Strip any newline added from userspace. */ - stripped = strstrip(buf); - len = strlen(stripped); - } - if (len > INQUIRY_MODEL_LEN) { + len = strscpy(buf, page, sizeof(buf)); + if (len == -E2BIG) { pr_err("Emulated T10 Vendor exceeds INQUIRY_MODEL_LEN: " __stringify(INQUIRY_MODEL_LEN) "\n"); @@ -1437,16 +1427,11 @@ static ssize_t target_wwn_revision_store(struct config_item *item, /* +2 to allow for a trailing (stripped) '\n' and null-terminator */ unsigned char buf[INQUIRY_REVISION_LEN + 2]; char *stripped = NULL; - size_t len; + ssize_t len; ssize_t ret; - len = strlcpy(buf, page, sizeof(buf)); - if (len < sizeof(buf)) { - /* Strip any newline added from userspace. */ - stripped = strstrip(buf); - len = strlen(stripped); - } - if (len > INQUIRY_REVISION_LEN) { + len = strscpy(buf, page, sizeof(buf)); + if (len == -E2BIG) { pr_err("Emulated T10 Revision exceeds INQUIRY_REVISION_LEN: " __stringify(INQUIRY_REVISION_LEN) "\n");
The strlcpy() reads the entire source buffer first, it is dangerous if the source buffer lenght is unbounded or possibility non NULL-terminated. It can lead to linear read overflows, crashes, etc... As recommended in the deprecated interfaces [1], it should be replaced by strscpy. This commit replaces all calls to strlcpy that handle the return values by the corresponding strscpy calls with new handling of the return values (as it is quite different between the two functions). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy Signed-off-by: Romain Perier <romain.perier@gmail.com> --- drivers/target/target_core_configfs.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)