Re: [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
On Wed, Apr 10, 2024 at 5:31 AM Kees Cook wrote:
>
> Another ambiguous use of strncpy() is to copy from strings that may not
> be NUL-terminated. These cases depend on having the destination buffer
> be explicitly larger than the source buffer's maximum size, having
> the size of the copy exactly match the source buffer's maximum size,
> and for the destination buffer to get explicitly NUL terminated.
>
> This usually happens when parsing protocols or hardware character arrays
> that are not guaranteed to be NUL-terminated. The code pattern is
> effectively this:
>
> char dest[sizeof(src) + 1];
>
> strncpy(dest, src, sizeof(src));
> dest[sizeof(dest) - 1] = '\0';
>
> In practice it usually looks like:
>
> struct from_hardware {
> ...
> char name[HW_NAME_SIZE] __nonstring;
> ...
> };
>
> struct from_hardware *p = ...;
> char name[HW_NAME_SIZE + 1];
>
> strncpy(name, p->name, HW_NAME_SIZE);
> name[NW_NAME_SIZE] = '\0';
>
> This cannot be replaced with:
>
> strscpy(name, p->name, sizeof(name));
>
> because p->name is smaller and not NUL-terminated, so FORTIFY will
> trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be
> replaced with:
>
> strscpy(name, p->name, sizeof(p->name));
>
> because then "name" may contain a 1 character early truncation of
> p->name.
>
> Provide an unambiguous interface for converting a maybe not-NUL-terminated
> string to a NUL-terminated string, with compile-time buffer size checking
> so that it can never fail at runtime: memtostr() and memtostr_pad(). Also
> add KUnit tests for both.
Obvious question, why can't strscpy() be fixed for this corner case?
--
With Best Regards,
Andy Shevchenko
[PATCH 5/5] scsi: qla2xxx: Avoid possible run-time warning with long model_num
The prior strlcpy() replacement of strncpy() here (which was
later replaced with strscpy()) expected pinfo->model_num (and
pinfo->model_description) to be NUL-terminated, but it is possible
it was not, as the code pattern here shows vha->hw->model_number (and
vha->hw->model_desc) being exactly 1 character larger, and the replaced
strncpy() was copying only up to the size of the source character
array. Replace this with memtostr(), which is the unambiguous way to
convert a maybe not-NUL-terminated character array into a NUL-terminated
string.
Fixes: 527e9b704c3d ("scsi: qla2xxx: Use memcpy() and strlcpy() instead of
strcpy() and strncpy()")
Signed-off-by: Kees Cook
---
Cc: Bart Van Assche
Cc: Nilesh Javali
Cc: gr-qlogic-storage-upstr...@marvell.com
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/qla2xxx/qla_mr.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index 083f94e43fba..82a7e21ddc83 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -1909,10 +1909,8 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport,
uint16_t fx_type)
if (fx_type == FXDISC_GET_CONFIG_INFO) {
struct config_info_data *pinfo =
(struct config_info_data *) fdisc->u.fxiocb.rsp_addr;
- strscpy(vha->hw->model_number, pinfo->model_num,
- ARRAY_SIZE(vha->hw->model_number));
- strscpy(vha->hw->model_desc, pinfo->model_description,
- ARRAY_SIZE(vha->hw->model_desc));
+ memtostr(vha->hw->model_number, pinfo->model_num);
+ memtostr(vha->hw->model_desc, pinfo->model_description);
memcpy(>hw->mr.symbolic_name, pinfo->symbolic_name,
sizeof(vha->hw->mr.symbolic_name));
memcpy(>hw->mr.serial_num, pinfo->serial_num,
--
2.34.1
[PATCH 4/5] scsi: mpi3mr: Avoid possible run-time warning with long manufacturer strings
The prior use of strscpy() here expected the manufacture_reply strings to
be NUL-terminated, but it is possible they are not, as the code pattern
here shows, e.g., edev->vendor_id being exactly 1 character larger than
manufacture_reply->vendor_id, and the strscpy() was copying only up to
the size of the source character array. Replace this with memtostr(),
which is the unambiguous way to convert a maybe not-NUL-terminated
character array into a NUL-terminated string.
Fixes: 2bd37e284914 ("scsi: mpi3mr: Add framework to issue MPT transport cmds")
Signed-off-by: Kees Cook
---
Cc: Sreekanth Reddy
Cc: Sathya Prakash Veerichetty
Cc: Kashyap Desai
Cc: Sumit Saxena
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: mpi3mr-linuxdrv@broadcom.com
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c
b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index dabb91f0f75d..3caceddb864a 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -211,17 +211,13 @@ static int mpi3mr_report_manufacture(struct mpi3mr_ioc
*mrioc,
goto out;
}
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
-SAS_EXPANDER_VENDOR_ID_LEN);
- strscpy(edev->product_id, manufacture_reply->product_id,
-SAS_EXPANDER_PRODUCT_ID_LEN);
- strscpy(edev->product_rev, manufacture_reply->product_rev,
-SAS_EXPANDER_PRODUCT_REV_LEN);
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format & 1;
if (edev->level) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
-SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN);
+ memtostr(edev->component_vendor_id,
+manufacture_reply->component_vendor_id);
tmp = (u8 *)_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
[PATCH 2/5] scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings
The prior strscpy() replacement of strncpy() here expected the
manufacture_reply strings to be NUL-terminated, but it is possible
they are not, as the code pattern here shows, e.g., edev->vendor_id
being exactly 1 character larger than manufacture_reply->vendor_id,
and the replaced strncpy() was copying only up to the size of the
source character array. Replace this with memtostr(), which is the
unambiguous way to convert a maybe not-NUL-terminated character array
into a NUL-terminated string.
Reported-by: Charles Bertsch
Closes:
https://lore.kernel.org/all/5445ba0f-3e27-4d43-a9ba-0cc22ada2...@cox.net/
Fixes: 45e833f0e5bb ("scsi: message: fusion: Replace deprecated strncpy() with
strscpy()")
Signed-off-by: Kees Cook
---
Cc: Justin Stitt
Cc: Sathya Prakash
Cc: Sreekanth Reddy
Cc: Suganath Prabu Subramani
Cc: mpt-fusionlinux@broadcom.com
Cc: linux-s...@vger.kernel.org
---
drivers/message/fusion/mptsas.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 300f8e955a53..0f80c840afc3 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2964,17 +2964,13 @@ mptsas_exp_repmanufacture_info(MPT_ADAPTER *ioc,
goto out_free;
manufacture_reply = data_out + sizeof(struct rep_manu_request);
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
- sizeof(edev->vendor_id));
- strscpy(edev->product_id, manufacture_reply->product_id,
- sizeof(edev->product_id));
- strscpy(edev->product_rev, manufacture_reply->product_rev,
- sizeof(edev->product_rev));
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format;
if (manufacture_reply->sas_format) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
- sizeof(edev->component_vendor_id));
+ memtostr(edev->component_vendor_id,
+manufacture_reply->component_vendor_id);
tmp = (u8 *)_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
[PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
Hi, Another code pattern using the gloriously ambiguous strncpy() function was turning maybe not-NUL-terminated character arrays into NUL-terminated strings. In these cases, when strncpy() is replaced with strscpy() it creates a situation where if the non-terminated string takes up the entire character array (i.e. it is not terminated) run-time warnings from CONFIG_FORTIFY_SOURCE will trip, since strscpy() was expecting to find a NUL-terminated source but checking for the NUL would walk off the end of the source buffer. In doing an instrumented build of the kernel to find these cases, it seems it was almost entirely a code pattern used in the SCSI subsystem, so the creation of the new helper, memtostr(), can land via the SCSI tree. And, as it turns out, inappropriate conversions have been happening for several years now, some of which even moved through strlcpy() first (and were never noticed either). This series fixes all 4 of the instances I could find in the SCSI subsystem. Thanks, -Kees Kees Cook (5): string.h: Introduce memtostr() and memtostr_pad() scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings scsi: mpt3sas: Avoid possible run-time warning with long manufacturer strings scsi: mpi3mr: Avoid possible run-time warning with long manufacturer strings scsi: qla2xxx: Avoid possible run-time warning with long model_num drivers/message/fusion/mptsas.c | 14 +++ drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +++ drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++ drivers/scsi/qla2xxx/qla_mr.c| 6 +-- include/linux/string.h | 49 lib/strscpy_kunit.c | 26 + 7 files changed, 93 insertions(+), 32 deletions(-) -- 2.34.1
[PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
Another ambiguous use of strncpy() is to copy from strings that may not
be NUL-terminated. These cases depend on having the destination buffer
be explicitly larger than the source buffer's maximum size, having
the size of the copy exactly match the source buffer's maximum size,
and for the destination buffer to get explicitly NUL terminated.
This usually happens when parsing protocols or hardware character arrays
that are not guaranteed to be NUL-terminated. The code pattern is
effectively this:
char dest[sizeof(src) + 1];
strncpy(dest, src, sizeof(src));
dest[sizeof(dest) - 1] = '\0';
In practice it usually looks like:
struct from_hardware {
...
char name[HW_NAME_SIZE] __nonstring;
...
};
struct from_hardware *p = ...;
char name[HW_NAME_SIZE + 1];
strncpy(name, p->name, HW_NAME_SIZE);
name[NW_NAME_SIZE] = '\0';
This cannot be replaced with:
strscpy(name, p->name, sizeof(name));
because p->name is smaller and not NUL-terminated, so FORTIFY will
trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be
replaced with:
strscpy(name, p->name, sizeof(p->name));
because then "name" may contain a 1 character early truncation of
p->name.
Provide an unambiguous interface for converting a maybe not-NUL-terminated
string to a NUL-terminated string, with compile-time buffer size checking
so that it can never fail at runtime: memtostr() and memtostr_pad(). Also
add KUnit tests for both.
Signed-off-by: Kees Cook
---
Cc: Justin Stitt
Cc: Andy Shevchenko
Cc: linux-hardening@vger.kernel.org
---
include/linux/string.h | 49 ++
lib/strscpy_kunit.c| 26 ++
2 files changed, 75 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 793c27ad7c0d..bd42cf85a95b 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -424,6 +424,55 @@ void memcpy_and_pad(void *dest, size_t dest_len, const
void *src, size_t count,
memcpy(dest, src, strnlen(src, min(_src_len, _dest_len))); \
} while (0)
+/**
+ * memtostr - Copy a possibly non-NUL-term string to a NUL-term string
+ * @dest: Pointer to destination NUL-terminates string
+ * @src: Pointer to character array (likely marked as __nonstring)
+ *
+ * This is a replacement for strncpy() uses where the source is not
+ * a NUL-terminated string.
+ *
+ * Note that sizes of @dest and @src must be known at compile-time.
+ */
+#define memtostr(dest, src)do {\
+ const size_t _dest_len = __builtin_object_size(dest, 1);\
+ const size_t _src_len = __builtin_object_size(src, 1); \
+ const size_t _src_chars = strnlen(src, _src_len); \
+ const size_t _copy_len = min(_dest_len - 1, _src_chars);\
+ \
+ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||\
+!__builtin_constant_p(_src_len) || \
+_dest_len == 0 || _dest_len == (size_t)-1 || \
+_src_len == 0 || _src_len == (size_t)-1); \
+ memcpy(dest, src, _copy_len); \
+ dest[_copy_len] = '\0'; \
+} while (0)
+
+/**
+ * memtostr_pad - Copy a possibly non-NUL-term string to a NUL-term string
+ *with NUL padding in the destination
+ * @dest: Pointer to destination NUL-terminates string
+ * @src: Pointer to character array (likely marked as __nonstring)
+ *
+ * This is a replacement for strncpy() uses where the source is not
+ * a NUL-terminated string.
+ *
+ * Note that sizes of @dest and @src must be known at compile-time.
+ */
+#define memtostr_pad(dest, src)do {
\
+ const size_t _dest_len = __builtin_object_size(dest, 1);\
+ const size_t _src_len = __builtin_object_size(src, 1); \
+ const size_t _src_chars = strnlen(src, _src_len); \
+ const size_t _copy_len = min(_dest_len - 1, _src_chars);\
+ \
+ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||\
+!__builtin_constant_p(_src_len) || \
+_dest_len == 0 || _dest_len == (size_t)-1 || \
+_src_len == 0 || _src_len == (size_t)-1); \
+ memcpy(dest, src, _copy_len); \
+ memset([_copy_len], 0, _dest_len - _copy_len); \
+} while (0)
+
/**
* memset_after - Set a value after a struct member to the end of a struct
*
diff --git a/lib/strscpy_kunit.c b/lib/strscpy_kunit.c
index a6b6344354ed..ac0b5d1678b3 100644
--- a/lib/strscpy_kunit.c
+++
[PATCH 3/5] scsi: mpt3sas: Avoid possible run-time warning with long manufacturer strings
The prior strscpy() replacement of strncpy() here expected the
manufacture_reply strings to be NUL-terminated, but it is possible
they are not, as the code pattern here shows, e.g., edev->vendor_id
being exactly 1 character larger than manufacture_reply->vendor_id,
and the replaced strncpy() was copying only up to the size of the
source character array. Replace this with memtostr(), which is the
unambiguous way to convert a maybe not-NUL-terminated character array
into a NUL-terminated string.
Fixes: b7e9712a02e8 ("scsi: mpt3sas: Replace deprecated strncpy() with
strscpy()")
Signed-off-by: Kees Cook
---
Cc: Justin Stitt
Cc: Sathya Prakash
Cc: Sreekanth Reddy
Cc: Suganath Prabu Subramani
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: mpt-fusionlinux@broadcom.com
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +-
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 258647fc6bdd..1320e06727df 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -4774,7 +4774,7 @@ _base_display_ioc_capabilities(struct MPT3SAS_ADAPTER
*ioc)
char desc[17] = {0};
u32 iounit_pg1_flags;
- strscpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
+ memtostr(desc, ioc->manu_pg0.ChipName);
ioc_info(ioc, "%s: FWVersion(%02d.%02d.%02d.%02d),
ChipRevision(0x%02x)\n",
desc,
(ioc->facts.FWVersion.Word & 0xFF00) >> 24,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 76f9a9177198..d84413b77d84 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -458,17 +458,13 @@ _transport_expander_report_manufacture(struct
MPT3SAS_ADAPTER *ioc,
goto out;
manufacture_reply = data_out + sizeof(struct rep_manu_request);
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
- sizeof(edev->vendor_id));
- strscpy(edev->product_id, manufacture_reply->product_id,
- sizeof(edev->product_id));
- strscpy(edev->product_rev, manufacture_reply->product_rev,
- sizeof(edev->product_rev));
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format & 1;
if (edev->level) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
- sizeof(edev->component_vendor_id));
+ memtostr(edev->component_vendor_id,
+manufacture_reply->component_vendor_id);
tmp = (u8 *)_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
Re: [PATCH] xfs: xattr: replace strncpy and check for truncation
On Tue, Apr 09, 2024 at 05:27:34PM -0700, Justin Stitt wrote:
> On Tue, Apr 9, 2024 at 5:23 PM Justin Stitt wrote:
> >
> > Hi,
> >
> > On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig wrote:
> > >
> > > On Fri, Apr 05, 2024 at 07:45:08PM +, Justin Stitt wrote:
> > > > - memcpy(offset, prefix, prefix_len);
> > > > - offset += prefix_len;
> > > > - strncpy(offset, (char *)name, namelen); /* real
> > > > name */
> > > > - offset += namelen;
> > > > - *offset = '\0';
> > > > +
> > > > + combined_len = prefix_len + namelen;
> > > > +
> > > > + /* plus one byte for \0 */
> > > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix,
> > > > name);
> > > > +
> > > > + if (actual_len < combined_len)
> > >
> > > Shouldn't this be a != ?
> >
> > I guess it could be. It's a truncation check so I figured just
> > checking if the amount of bytes actually copied was less than the
> > total would suffice.
> >
> > >
> > > That being said I think this is actually wrong - the attr names are
> > > not NULL-terminated on disk, which is why we have the explicit
> > > zero terminataion above.
> >
> > Gotcha, in which case we could use the "%.*s" format specifier which
> > allows for a length argument. Does something like this look better?
> >
> > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > index 364104e1b38a..1b7e886e0f29 100644
> > --- a/fs/xfs/xfs_xattr.c
> > +++ b/fs/xfs/xfs_xattr.c
> > @@ -206,6 +206,7 @@ __xfs_xattr_put_listent(
> > {
> > char *offset;
> > int arraytop;
> > + size_t combined_len, actual_len;
> >
> > if (context->count < 0 || context->seen_enough)
> > return;
> > @@ -220,11 +221,16 @@ __xfs_xattr_put_listent(
> > return;
> > }
> > offset = context->buffer + context->count;
> > - memcpy(offset, prefix, prefix_len);
> > - offset += prefix_len;
> > - strncpy(offset, (char *)name, namelen); /* real name */
> > - offset += namelen;
> > - *offset = '\0';
> > +
> > + combined_len = prefix_len + namelen;
> > +
> > + /* plus one byte for \0 */
> > + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s",
> > +prefix_len, prefix, namelen, name);
> > +
> > + if (actual_len < combined_len)
> > + xfs_warn(context->dp->i_mount,
> > + "cannot completely copy context buffer resulting in truncation");
> >
> > compute_size:
> > context->count += prefix_len + namelen + 1;
> > ---
>
> I copy pasted from vim -> gmail and it completely ate all my tabs.
> When I actually send the new patch, if needed, it will be formatted
> correctly :)
Yeah, the "%.*s" version looks better.
> >
> >
> >
> > >
> > > How was this tested?
> >
> > With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/
> >
> > but using scripts + image from: https://github.com/tytso/xfstests-bld
> >
> > here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the
> > 5 default ones (I think?):
> >
> > |Ran: generic/475 generic/476 generic/521 generic/522 generic/642
> > |Passed all 5 tests
Would you mind adding "-g attr,label" into the mix so that you're running all
the functional tests for xattr and fs label functionality?
--D
> >
> > Thanks
> > Justin
>
Re: [PATCH] xfs: xattr: replace strncpy and check for truncation
On Tue, Apr 9, 2024 at 5:23 PM Justin Stitt wrote:
>
> Hi,
>
> On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig wrote:
> >
> > On Fri, Apr 05, 2024 at 07:45:08PM +, Justin Stitt wrote:
> > > - memcpy(offset, prefix, prefix_len);
> > > - offset += prefix_len;
> > > - strncpy(offset, (char *)name, namelen); /* real
> > > name */
> > > - offset += namelen;
> > > - *offset = '\0';
> > > +
> > > + combined_len = prefix_len + namelen;
> > > +
> > > + /* plus one byte for \0 */
> > > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix,
> > > name);
> > > +
> > > + if (actual_len < combined_len)
> >
> > Shouldn't this be a != ?
>
> I guess it could be. It's a truncation check so I figured just
> checking if the amount of bytes actually copied was less than the
> total would suffice.
>
> >
> > That being said I think this is actually wrong - the attr names are
> > not NULL-terminated on disk, which is why we have the explicit
> > zero terminataion above.
>
> Gotcha, in which case we could use the "%.*s" format specifier which
> allows for a length argument. Does something like this look better?
>
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 364104e1b38a..1b7e886e0f29 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -206,6 +206,7 @@ __xfs_xattr_put_listent(
> {
> char *offset;
> int arraytop;
> + size_t combined_len, actual_len;
>
> if (context->count < 0 || context->seen_enough)
> return;
> @@ -220,11 +221,16 @@ __xfs_xattr_put_listent(
> return;
> }
> offset = context->buffer + context->count;
> - memcpy(offset, prefix, prefix_len);
> - offset += prefix_len;
> - strncpy(offset, (char *)name, namelen); /* real name */
> - offset += namelen;
> - *offset = '\0';
> +
> + combined_len = prefix_len + namelen;
> +
> + /* plus one byte for \0 */
> + actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s",
> +prefix_len, prefix, namelen, name);
> +
> + if (actual_len < combined_len)
> + xfs_warn(context->dp->i_mount,
> + "cannot completely copy context buffer resulting in truncation");
>
> compute_size:
> context->count += prefix_len + namelen + 1;
> ---
I copy pasted from vim -> gmail and it completely ate all my tabs.
When I actually send the new patch, if needed, it will be formatted
correctly :)
>
>
>
> >
> > How was this tested?
>
> With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/
>
> but using scripts + image from: https://github.com/tytso/xfstests-bld
>
> here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the
> 5 default ones (I think?):
>
> |Ran: generic/475 generic/476 generic/521 generic/522 generic/642
> |Passed all 5 tests
>
> Thanks
> Justin
Re: [PATCH] xfs: xattr: replace strncpy and check for truncation
Hi,
On Tue, Apr 9, 2024 at 6:32 AM Christoph Hellwig wrote:
>
> On Fri, Apr 05, 2024 at 07:45:08PM +, Justin Stitt wrote:
> > - memcpy(offset, prefix, prefix_len);
> > - offset += prefix_len;
> > - strncpy(offset, (char *)name, namelen); /* real name
> > */
> > - offset += namelen;
> > - *offset = '\0';
> > +
> > + combined_len = prefix_len + namelen;
> > +
> > + /* plus one byte for \0 */
> > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix,
> > name);
> > +
> > + if (actual_len < combined_len)
>
> Shouldn't this be a != ?
I guess it could be. It's a truncation check so I figured just
checking if the amount of bytes actually copied was less than the
total would suffice.
>
> That being said I think this is actually wrong - the attr names are
> not NULL-terminated on disk, which is why we have the explicit
> zero terminataion above.
Gotcha, in which case we could use the "%.*s" format specifier which
allows for a length argument. Does something like this look better?
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 364104e1b38a..1b7e886e0f29 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -206,6 +206,7 @@ __xfs_xattr_put_listent(
{
char *offset;
int arraytop;
+ size_t combined_len, actual_len;
if (context->count < 0 || context->seen_enough)
return;
@@ -220,11 +221,16 @@ __xfs_xattr_put_listent(
return;
}
offset = context->buffer + context->count;
- memcpy(offset, prefix, prefix_len);
- offset += prefix_len;
- strncpy(offset, (char *)name, namelen); /* real name */
- offset += namelen;
- *offset = '\0';
+
+ combined_len = prefix_len + namelen;
+
+ /* plus one byte for \0 */
+ actual_len = scnprintf(offset, combined_len + 1, "%.*s%.*s",
+prefix_len, prefix, namelen, name);
+
+ if (actual_len < combined_len)
+ xfs_warn(context->dp->i_mount,
+ "cannot completely copy context buffer resulting in truncation");
compute_size:
context->count += prefix_len + namelen + 1;
---
>
> How was this tested?
With https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/about/
but using scripts + image from: https://github.com/tytso/xfstests-bld
here's the output log: https://pastebin.com/V2gFhbNZ wherein I ran the
5 default ones (I think?):
|Ran: generic/475 generic/476 generic/521 generic/522 generic/642
|Passed all 5 tests
Thanks
Justin
RE: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
> Do ECC servers wipe their RAM by default? I know that if you build with > CONFIG_RESET_ATTACK_MITIGATION=y on an EFI system that supports the > MemoryOverwriteRequestControl EFI variable you'll get a RAM wipe... I know that after I've been running RAS tests that inject ECC errors into thousands of pages, those errors all disappear after a reboot. I think some BIOS have options to speed up boot by skipping memory initialization. I don't know if anyone makes that the default mode. -Tony
Re: [POC][RFC][PATCH 1/2] mm/x86: Add wildcard * option as memmap=nn*align:name
On Tue, Apr 09, 2024 at 07:11:56PM -0400, Steven Rostedt wrote: > On Tue, 9 Apr 2024 15:23:07 -0700 > Kees Cook wrote: > > > Do we need to involve e820 at all? I think it might be possible to just > > have pstore call request_mem_region() very early? Or does KASLR make > > that unstable? > > Yeah, would that give the same physical memory each boot, and can we > guarantee that KASLR will not map the kernel over the previous location? Hm, no, for physical memory it needs to get excluded very early, which means e820. So, yeah, your proposal makes sense. I'm not super excited about this be x86-only though. What does arm64 for for memmap? -- Kees Cook
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On Tue, Apr 09, 2024 at 10:25:33PM +, Luck, Tony wrote: > >> I forgot to mention that this makes it trivial for any machine that doesn't > >> clear memory on soft-reboot, to enable console ramoops (to have access to > >> the last boot dmesg without needing serial). > >> > >> I tested this on a couple of my test boxes and on QEMU, and it works rather > >> well. > > > > I've long wanted a "stable for this machine and kernel" memory region > > like this for pstore. It would make testing much easier. > > Which systems does this work on? I'd assume that servers (and anything > else with ECC memory) would nuke contents while resetting ECC to clean > state. Do ECC servers wipe their RAM by default? I know that if you build with CONFIG_RESET_ATTACK_MITIGATION=y on an EFI system that supports the MemoryOverwriteRequestControl EFI variable you'll get a RAM wipe... -- Kees Cook
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On Tue, 9 Apr 2024 22:25:33 + "Luck, Tony" wrote: > >> I forgot to mention that this makes it trivial for any machine that doesn't > >> clear memory on soft-reboot, to enable console ramoops (to have access to > >> the last boot dmesg without needing serial). > >> > >> I tested this on a couple of my test boxes and on QEMU, and it works rather > >> well. > > > > I've long wanted a "stable for this machine and kernel" memory region > > like this for pstore. It would make testing much easier. > > Which systems does this work on? I'd assume that servers (and anything > else with ECC memory) would nuke contents while resetting ECC to clean > state. > Well I tested it on a couple of chromebooks, a test box and a laptop (as well as QEMU). I know that ramoops has an ecc option. I'm guessing that would help here (but I'd have to defer to others to answer that). -- Steve
Re: [POC][RFC][PATCH 2/2] pstore/ramoops: Add ramoops.mem_name= command line option
On Tue, 9 Apr 2024 15:18:45 -0700
Kees Cook wrote:
> > @@ -914,6 +919,19 @@ static void __init ramoops_register_dummy(void)
> > {
> > struct ramoops_platform_data pdata;
> >
> > +#ifndef MODULE
> > + /* Only allowed when builtin */
>
> Why only when builtin?
Well, because the memory table that maps the found physical memory to a
lable is marked as __initdata, and will not be available after boot. If you
wanted it for a module, you would need some builtin code to find it.
>
> > + if (mem_name) {
> > + u64 start;
> > + u64 size;
> > +
> > + if (memmap_named(mem_name, , )) {
> > + mem_address = start;
> > + mem_size = size;
> > + }
> > + }
> > +#endif
>
> Otherwise this looks good, though I'd prefer some comments about what's
> happening here.
>
> (And in retrospect, separately, I probably need to rename "dummy" to
> "commandline" or something, since it's gathering valid settings here...)
Yeah, that was a bit confusing. I kept thinking "is this function stable?".
-- Steve
Re: [POC][RFC][PATCH 1/2] mm/x86: Add wildcard * option as memmap=nn*align:name
On Tue, 9 Apr 2024 15:23:07 -0700 Kees Cook wrote: > Do we need to involve e820 at all? I think it might be possible to just > have pstore call request_mem_region() very early? Or does KASLR make > that unstable? Yeah, would that give the same physical memory each boot, and can we guarantee that KASLR will not map the kernel over the previous location? -- Steve
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
> On Apr 10, 2024, at 3:55 AM, Luck, Tony wrote: > > >> >>> I forgot to mention that this makes it trivial for any machine that doesn't >>> clear memory on soft-reboot, to enable console ramoops (to have access to >>> the last boot dmesg without needing serial). >>> >>> I tested this on a couple of my test boxes and on QEMU, and it works rather >>> well. >> >> I've long wanted a "stable for this machine and kernel" memory region >> like this for pstore. It would make testing much easier. > > Which systems does this work on? I'd assume that servers (and anything > else with ECC memory) would nuke contents while resetting ECC to clean > state. If that were the case universally, then ramoops pstore backend would not work either? And yet we get the last kernel logs via the pstore for many years now, on embedded-ish devices. From my reading, ECC-enabled DRAM is not present on lots of systems and IIRC, pstore ramoops has its own ECC. Or did I miss a recent trend with ECC-enabled DRAM? - Joel > > -Tony
RE: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
>> I forgot to mention that this makes it trivial for any machine that doesn't >> clear memory on soft-reboot, to enable console ramoops (to have access to >> the last boot dmesg without needing serial). >> >> I tested this on a couple of my test boxes and on QEMU, and it works rather >> well. > > I've long wanted a "stable for this machine and kernel" memory region > like this for pstore. It would make testing much easier. Which systems does this work on? I'd assume that servers (and anything else with ECC memory) would nuke contents while resetting ECC to clean state. -Tony
Re: [POC][RFC][PATCH 1/2] mm/x86: Add wildcard * option as memmap=nn*align:name
On Tue, Apr 09, 2024 at 05:02:55PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> In order to allow for requesting a memory region that can be used for
> things like pstore on multiple machines where the memory is not the same,
> add a new option to the memmap=nn$ kernel command line.
>
> The memmap=nn$addr will reserve nn amount of memory at the physical
> address addr. To use this, one must know the physical memory layout and
> know where usable memory exists in the physical layout.
>
> Add a '*' option that will assign memory by looking for a range that can
> fit the given size and alignment. It will start at the high addresses, and
> then work its way down.
>
> The format is: memmap=nn*align:name
>
> Where it will find nn amount of memory at the given alignment of align.
> The name field is to allow another subsystem to retrieve where the memory
> was found. For example:
>
> memmap=12M*4096:oops ramoops.mem_name=oops
>
> Where ramoops.mem_name will tell ramoops that memory was reserved for it
> via the wildcard '*' option and it can find it by calling:
>
> if (memmap_named("oops", , )) {
> // start holds the start address and size holds the size given
>
> Signed-off-by: Steven Rostedt (Google)
> ---
> arch/x86/kernel/e820.c | 91 ++
> include/linux/mm.h | 2 +
> mm/memory.c| 7
> 3 files changed, 100 insertions(+)
Do we need to involve e820 at all? I think it might be possible to just
have pstore call request_mem_region() very early? Or does KASLR make
that unstable?
--
Kees Cook
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On Tue, Apr 09, 2024 at 05:23:58PM -0400, Steven Rostedt wrote: > On Tue, 09 Apr 2024 17:02:54 -0400 > Steven Rostedt wrote: > > > memmap=12M*4096:oops ramoops.mem_name=oops > > I forgot to mention that this makes it trivial for any machine that doesn't > clear memory on soft-reboot, to enable console ramoops (to have access to > the last boot dmesg without needing serial). > > I tested this on a couple of my test boxes and on QEMU, and it works rather > well. I've long wanted a "stable for this machine and kernel" memory region like this for pstore. It would make testing much easier. -- Kees Cook
Re: [POC][RFC][PATCH 2/2] pstore/ramoops: Add ramoops.mem_name= command line option
On Tue, Apr 09, 2024 at 05:02:56PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> Add a method to find a region specified by memmap=nn*align:name for
> ramoops. Adding a kernel command line parameter:
>
> memmap=12M*4096:oops ramoops.mem_name=oops
>
> Will use the size and location defined by the memmap parameter where it
> finds the memory and labels it "oops". The "oops" in the ramoops option
> is used to search for it.
>
> This allows for arbitrary RAM to be used for ramoops if it is known that
> the memory is not cleared on kernel crashes or soft reboots.
>
> Signed-off-by: Steven Rostedt (Google)
> ---
> fs/pstore/ram.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index b1a455f42e93..c200388399fb 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -50,6 +50,11 @@ module_param_hw(mem_address, ullong, other, 0400);
> MODULE_PARM_DESC(mem_address,
> "start of reserved RAM used to store oops/panic logs");
>
> +static char *mem_name;
> +module_param_named(mem_name, mem_name, charp, 0400);
> +MODULE_PARM_DESC(mem_name,
> + "name of kernel param that holds addr (builtin only)");
> +
> static ulong mem_size;
> module_param(mem_size, ulong, 0400);
> MODULE_PARM_DESC(mem_size,
> @@ -914,6 +919,19 @@ static void __init ramoops_register_dummy(void)
> {
> struct ramoops_platform_data pdata;
>
> +#ifndef MODULE
> + /* Only allowed when builtin */
Why only when builtin?
> + if (mem_name) {
> + u64 start;
> + u64 size;
> +
> + if (memmap_named(mem_name, , )) {
> + mem_address = start;
> + mem_size = size;
> + }
> + }
> +#endif
Otherwise this looks good, though I'd prefer some comments about what's
happening here.
(And in retrospect, separately, I probably need to rename "dummy" to
"commandline" or something, since it's gathering valid settings here...)
--
Kees Cook
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
On Tue, 9 Apr 2024 18:01:40 +0100 Edward Cree wrote: > > Shared branch would be good. Ed has some outstanding patches > > to refactor the ethtool RSS API. > > For the record I am extremely unlikely to have time to get those > done this cycle :( > Though in any case fwiw it doesn't look like this series touches > anything that would conflict; mana doesn't appear to support > custom RSS contexts and besides the changes are well away from > the ethtool API handling. Better safe than sorry, since the change applies cleanly on an -rc tag having it applied to both trees should be very little extra work.
Re: [POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
On Tue, 09 Apr 2024 17:02:54 -0400 Steven Rostedt wrote: > memmap=12M*4096:oops ramoops.mem_name=oops I forgot to mention that this makes it trivial for any machine that doesn't clear memory on soft-reboot, to enable console ramoops (to have access to the last boot dmesg without needing serial). I tested this on a couple of my test boxes and on QEMU, and it works rather well. -- Steve
[POC][RFC][PATCH 1/2] mm/x86: Add wildcard * option as memmap=nn*align:name
From: "Steven Rostedt (Google)"
In order to allow for requesting a memory region that can be used for
things like pstore on multiple machines where the memory is not the same,
add a new option to the memmap=nn$ kernel command line.
The memmap=nn$addr will reserve nn amount of memory at the physical
address addr. To use this, one must know the physical memory layout and
know where usable memory exists in the physical layout.
Add a '*' option that will assign memory by looking for a range that can
fit the given size and alignment. It will start at the high addresses, and
then work its way down.
The format is: memmap=nn*align:name
Where it will find nn amount of memory at the given alignment of align.
The name field is to allow another subsystem to retrieve where the memory
was found. For example:
memmap=12M*4096:oops ramoops.mem_name=oops
Where ramoops.mem_name will tell ramoops that memory was reserved for it
via the wildcard '*' option and it can find it by calling:
if (memmap_named("oops", , )) {
// start holds the start address and size holds the size given
Signed-off-by: Steven Rostedt (Google)
---
arch/x86/kernel/e820.c | 91 ++
include/linux/mm.h | 2 +
mm/memory.c| 7
3 files changed, 100 insertions(+)
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 6f1b379e3b38..a8831ef30c73 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -64,6 +64,61 @@ struct e820_table *e820_table __refdata
= _table_init;
struct e820_table *e820_table_kexec __refdata =
_table_kexec_init;
struct e820_table *e820_table_firmware __refdata =
_table_firmware_init;
+/* For wildcard memory requests, have a table to find them later */
+#define E820_MAX_MAPS 8
+#define E820_MAP_NAME_SIZE 16
+struct e820_mmap_map {
+ charname[E820_MAP_NAME_SIZE];
+ u64 start;
+ u64 size;
+};
+static struct e820_mmap_map e820_mmap_list[E820_MAX_MAPS] __initdata;
+static int e820_mmap_size __initdata;
+
+/* Add wildcard region with a lookup name */
+static int __init e820_add_mmap(u64 start, u64 size, const char *name)
+{
+ struct e820_mmap_map *map;
+
+ if (!name || !name[0] || strlen(name) >= E820_MAP_NAME_SIZE)
+ return -EINVAL;
+
+ if (e820_mmap_size >= E820_MAX_MAPS)
+ return -1;
+
+ map = _mmap_list[e820_mmap_size++];
+ map->start = start;
+ map->size = size;
+ strcpy(map->name, name);
+ return 0;
+}
+
+/**
+ * memmap_named - Find a wildcard region with a given name
+ * @name: The name that is attached to a wildcard region
+ * @start: If found, holds the start address
+ * @size: If found, holds the size of the address.
+ *
+ * Returns: 1 if found or 0 if not found.
+ */
+int __init memmap_named(const char *name, u64 *start, u64 *size)
+{
+ struct e820_mmap_map *map;
+ int i;
+
+ for (i = 0; i < e820_mmap_size; i++) {
+ map = _mmap_list[i];
+ if (!map->size)
+ continue;
+ if (strcmp(name, map->name) == 0) {
+ *start = map->start;
+ *size = map->size;
+ return 1;
+ }
+ }
+ return 0;
+}
+
/* For PCI or other memory-mapped resources */
unsigned long pci_mem_start = 0xaeedbabe;
#ifdef CONFIG_PCI
@@ -200,6 +255,29 @@ static void __init e820_print_type(enum e820_type type)
}
}
+/*
+ * Search for usable ram that can be reserved for a wildcard.
+ * Start at the highest memory and work down to lower memory.
+ */
+static s64 e820__region(u64 size, u64 align)
+{
+ u64 start;
+ int i;
+
+ for (i = e820_table->nr_entries; i >= 0; i--) {
+ if (e820_table->entries[i].type != E820_TYPE_RAM &&
+ e820_table->entries[i].type != E820_TYPE_RESERVED_KERN)
+ continue;
+
+ start = e820_table->entries[i].addr +
e820_table->entries[i].size;
+ start -= size;
+ start = ALIGN_DOWN(start, align);
+ if (start >= e820_table->entries[i].addr)
+ return start;
+ }
+ return -1;
+}
+
void __init e820__print_table(char *who)
{
int i;
@@ -944,6 +1022,19 @@ static int __init parse_memmap_one(char *p)
} else if (*p == '$') {
start_at = memparse(p+1, );
e820__range_add(start_at, mem_size, E820_TYPE_RESERVED);
+ } else if (*p == '*') {
+ u64 align;
+ /* Followed by alignment and ':' then the name */
+ align = memparse(p+1, );
+ start_at = e820__region(mem_size, align);
+ if ((s64)start_at < 0)
+ return -EINVAL;
+ if (*p != ':')
+
[POC][RFC][PATCH 0/2] pstore/mm/x86: Add wildcard memmap to map pstore consistently
Add wildcard option of reserving physical memory on kernel command line Background: In ChromeOS, we have 1 MB of pstore ramoops reserved so that we can extract dmesg output and some other information when a crash happens in the field. (This is only done when the user selects "Allow Google to collect data for improving the system"). But there are cases when there's a bug that requires more data to be retrieved to figure out what is happening. We would like to increase the pstore size, either temporarily, or maybe even permanently. The pstore on these devices are at a fixed location in RAM (as the RAM is not cleared on soft reboots nor crashes). The location is chosen by the BIOS (coreboot) and passed to the kernel via ACPI tables on x86. There's a driver that queries for this to initialize the pstore for ChromeOS: See drivers/platform/chrome/chromeos_pstore.c Problem: The problem is that, even though there's a process to change the kernel on these systems, and is done regularly to install updates, the firmware is updated much less frequently. Choosing the place in RAM also takes special care, and may be in a different address for different boards. Updating the size via firmware is a large effort and not something that many are willing to do for a temporary pstore size change. Requirement: Need a way to reserve memory that will be at a consistent location for every boot, if the kernel and system are the same. Does not need to work if rebooting to a different kernel, or if the system can change the memory layout between boots. The reserved memory can not be an hard coded address, as the same kernel / command line needs to run on several different machines. The picked memory reservation just needs to be the same for a given machine, but may be different for different machines. Solution: The solution I have come up with is to introduce a new "memmap=" kernel command line (for x86 and I would like something similar for ARM that uses device tree). As "memmap=" kernel command line parameter takes on several flavors already, I would like to introduce a new one. The "memmap=" kernel parameter is of the format of: memmap=nn[Xss] Where nn is the size, 'X' defines the flavor, and 'ss' usually a parameter to that flavor. The '$' flavor is to reserve physical memory where you could have: memmap=12M$0xb00 Where 12 megs of memory will be reserved at the address 0xb000. This memory will not be part of the memory used by the kernel's memory management system. (e.g. alloc_pages() and kmalloc() will not return memory in that location). I would like to introduce a "wildcard" flavor that is of the format: memmap=nn*align:label Where nn is the size of memory to reserve, the align is the alignment of that memory, and label is the way for other sub-systems to find that memory. This way the kernel command line could have: memmap=12M*4096:oops ramoops.mem_name=oops At boot up, the kernel will search for 12 megabytes in usable memory regions with an alignment of 4096. It will start at the highest regions and work its way down (for those old devices that want access to lower address DMA). When it finds a region, it will save it off in a small table and mark it with the "oops" label. Then the pstore ramoops sub-system could ask for that memory and location, and it will map itself there. This prototype allows for 8 different mappings (which may be overkill, 4 is probably plenty) with 16 byte size to store the label. The table lookup is only available until boot finishes, which means it is only available for builtin code and not for modules. I have tested this and it works for us to solve the above problem. We can update the kernel and command line and increase the size of pstore without needing to update the firmware, or knowing every memory layout of each board. I only tested this locally, it has not been tested in the field. Before doing anything, I am looking for feedback. Maybe I missed something. Perhaps there's a better way. Anyway, this is both a Proof of Concept as well as a Request for Comments. Thanks! Steven Rostedt (Google) (2): mm/x86: Add wildcard '*' option as memmap=nn*align:name pstore/ramoops: Add ramoops.mem_name= command line option arch/x86/kernel/e820.c | 91 ++ fs/pstore/ram.c| 18 ++ include/linux/mm.h | 2 ++ mm/memory.c| 7 4 files changed, 118 insertions(+)
[POC][RFC][PATCH 2/2] pstore/ramoops: Add ramoops.mem_name= command line option
From: "Steven Rostedt (Google)"
Add a method to find a region specified by memmap=nn*align:name for
ramoops. Adding a kernel command line parameter:
memmap=12M*4096:oops ramoops.mem_name=oops
Will use the size and location defined by the memmap parameter where it
finds the memory and labels it "oops". The "oops" in the ramoops option
is used to search for it.
This allows for arbitrary RAM to be used for ramoops if it is known that
the memory is not cleared on kernel crashes or soft reboots.
Signed-off-by: Steven Rostedt (Google)
---
fs/pstore/ram.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..c200388399fb 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -50,6 +50,11 @@ module_param_hw(mem_address, ullong, other, 0400);
MODULE_PARM_DESC(mem_address,
"start of reserved RAM used to store oops/panic logs");
+static char *mem_name;
+module_param_named(mem_name, mem_name, charp, 0400);
+MODULE_PARM_DESC(mem_name,
+ "name of kernel param that holds addr (builtin only)");
+
static ulong mem_size;
module_param(mem_size, ulong, 0400);
MODULE_PARM_DESC(mem_size,
@@ -914,6 +919,19 @@ static void __init ramoops_register_dummy(void)
{
struct ramoops_platform_data pdata;
+#ifndef MODULE
+ /* Only allowed when builtin */
+ if (mem_name) {
+ u64 start;
+ u64 size;
+
+ if (memmap_named(mem_name, , )) {
+ mem_address = start;
+ mem_size = size;
+ }
+ }
+#endif
+
/*
* Prepare a dummy platform data structure to carry the module
* parameters. If mem_size isn't set, then there are no module
--
2.43.0
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
On 09/04/2024 02:36, Jakub Kicinski wrote: > On Mon, 8 Apr 2024 14:07:30 +0300 Leon Romanovsky wrote: >> Jakub, do you want shared branch for this series or should I take >> everything through RDMA tree as netdev part is small enough? > > Shared branch would be good. Ed has some outstanding patches > to refactor the ethtool RSS API. For the record I am extremely unlikely to have time to get those done this cycle :( Though in any case fwiw it doesn't look like this series touches anything that would conflict; mana doesn't appear to support custom RSS contexts and besides the changes are well away from the ethtool API handling. -e
Re: [PATCH] xfs: replace deprecated strncpy with strscpy_pad
On Fri, Apr 05, 2024 at 07:52:27PM +, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> The current code has taken care of NUL-termination by memset()'ing
> @label. This is followed by a strncpy() to perform the string copy.
>
> Instead, use strscpy_pad() to get both 1) NUL-termination and 2)
> NUL-padding which is needed as this is copied out to userspace.
>
> Note that this patch uses the new 2-argument version of strscpy_pad
> introduced in Commit e6584c3964f2f ("string: Allow 2-argument
> strscpy()").
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
>
> Signed-off-by: Justin Stitt
> ---
> Split from
> https://lore.kernel.org/all/20240401-strncpy-fs-xfs-xfs_ioctl-c-v1-1-02b9feb19...@google.com/
> with feedback from Christoph H.
> ---
> fs/xfs/xfs_ioctl.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d0e2cec6210d..a1156a8b1e15 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1750,15 +1750,14 @@ xfs_ioc_getlabel(
> char__user *user_label)
> {
> struct xfs_sb *sbp = >m_sb;
> + /* 1 larger than sb_fname, for a trailing NUL char */
> charlabel[XFSLABEL_MAX + 1];
>
> /* Paranoia */
> BUILD_BUG_ON(sizeof(sbp->sb_fname) > FSLABEL_MAX);
>
> - /* 1 larger than sb_fname, so this ensures a trailing NUL char */
> - memset(label, 0, sizeof(label));
> spin_lock(>m_sb_lock);
> - strncpy(label, sbp->sb_fname, XFSLABEL_MAX);
> + strscpy_pad(label, sbp->sb_fname);
Is sbp->sb_fname itself NUL-terminated? This looks like another case of
needing the memtostr() helper?
-Kees
> spin_unlock(>m_sb_lock);
>
> if (copy_to_user(user_label, label, sizeof(label)))
>
> ---
> base-commit: c85af715cac0a951eea97393378e84bb49384734
> change-id: 20240405-strncpy-xfs-split1-a2c408b934c6
>
> Best regards,
> --
> Justin Stitt
>
>
--
Kees Cook
Re: [PATCH] xfs: xattr: replace strncpy and check for truncation
On Fri, Apr 05, 2024 at 07:45:08PM +, Justin Stitt wrote: > - memcpy(offset, prefix, prefix_len); > - offset += prefix_len; > - strncpy(offset, (char *)name, namelen); /* real name */ > - offset += namelen; > - *offset = '\0'; > + > + combined_len = prefix_len + namelen; > + > + /* plus one byte for \0 */ > + actual_len = scnprintf(offset, combined_len + 1, "%s%s", prefix, name); > + > + if (actual_len < combined_len) Shouldn't this be a != ? That being said I think this is actually wrong - the attr names are not NULL-terminated on disk, which is why we have the explicit zero terminataion above. How was this tested?
Re: [PATCH v3 0/3] RDMA/mana_ib: Add flex array to struct mana_cfg_rx_steer_req_v2
On Mon, Apr 08, 2024 at 06:36:57PM -0700, Jakub Kicinski wrote: > On Mon, 8 Apr 2024 14:07:30 +0300 Leon Romanovsky wrote: > > Jakub, do you want shared branch for this series or should I take > > everything through RDMA tree as netdev part is small enough? > > Shared branch would be good. Ed has some outstanding patches > to refactor the ethtool RSS API. Great, I'll wait for a day or two to give people time to review and then I'll create a shared branch. Thanks
Re: [PATCH v3] net: dsa: lan9303: use ethtool_puts() for lan9303_get_strings()
From: Justin Stitt
Date: Mon, 08 Apr 2024 21:01:57 +
> This pattern of strncpy with some pointer arithmetic setting fixed-sized
> intervals with string literal data is a bit weird so let's use
> ethtool_puts() as this has more obvious behavior and is less-error
> prone.
>
> Nicely, we also get to drop a usage of the now deprecated strncpy() [1].
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> Suggested-by: Alexander Lobakin
> ---
> Changes in v3:
> - use ethtool_puts over ethtool_sprintf
> - Link to v2:
> https://lore.kernel.org/r/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v2-1-feb452a53...@google.com
>
> Changes in v2:
> - use ethtool_sprintf (thanks Alexander)
> - Link to v1:
> https://lore.kernel.org/r/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v1-1-5a66c5381...@google.com
> ---
> drivers/net/dsa/lan9303-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index fcb20eac332a..04d3af0eb28e 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1007,14 +1007,14 @@ static const struct lan9303_mib_desc lan9303_mib[] = {
> static void lan9303_get_strings(struct dsa_switch *ds, int port,
> u32 stringset, uint8_t *data)
> {
> + u8 *buf = data;
> unsigned int u;
>
> if (stringset != ETH_SS_STATS)
> return;
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
> - ETH_GSTRING_LEN);
> + ethtool_puts(, lan9303_mib[u].name);
> }
The braces { } around ethtool_puts() aren't needed anymore.
> }
>
>
> ---
> base-commit: c85af715cac0a951eea97393378e84bb49384734
> change-id: 20231005-strncpy-drivers-net-dsa-lan9303-core-c-6386858e5c22
>
> Best regards,
> --
> Justin Stitt
Thanks,
Olek
Re: [PATCH] mtd: maps: sa1100-flash: Prefer struct_size over open coded arithmetic
On Sat, 2024-03-30 at 17:55:35 UTC, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> As the "info" variable is a pointer to "struct sa_info" and this
> structure ends in a flexible array:
>
> struct sa_info {
> [...]
> struct sa_subdev_info subdev[];
> };
>
> the preferred way in the kernel is to use the struct_size() helper to
> do the arithmetic instead of the calculation "size + size * count" in
> the kzalloc() function.
>
> This way, the code is more readable and safer.
>
> This code was detected with the help of Coccinelle, and audited and
> modified manually.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Signed-off-by: Erick Archer
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
mtd/next, thanks.
Miquel
