Re: [PATCH] fs: Set file_handle::handle_bytes before referencing file_handle::f_handle
On Wed 03-04-24 14:54:03, Kees Cook wrote:
> With adding __counted_by(handle_bytes) to struct file_handle, we need
> to explicitly set it in the one place it wasn't yet happening prior to
> accessing the flex array "f_handle".
>
> Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and
> use struct_size()")
> Signed-off-by: Kees Cook
OK, so this isn't really a functional bug AFAIU but the compiler will
wrongly complain we are accessing handle->f_handle beyond claimed array
size (because handle->handle_bytes == 0 at that point). Am I right? If
that's the case, please add a short comment explaining this (because it
looks odd we set handle->handle_bytes and then reset it a few lines later).
With the comment feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> Cc: Christian Brauner
> Cc: "Gustavo A. R. Silva"
> Cc: Alexander Viro
> Cc: Jan Kara
> Cc: Chuck Lever
> Cc: Jeff Layton
> Cc: Amir Goldstein
> Cc: linux-fsde...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> ---
> fs/fhandle.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 53ed54711cd2..08ec2340dd22 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
>GFP_KERNEL);
> if (!handle)
> return -ENOMEM;
> + handle->handle_bytes = f_handle.handle_bytes;
>
> /* convert handle size to multiple of sizeof(u32) */
> handle_dwords = f_handle.handle_bytes >> 2;
> --
> 2.34.1
>
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 1/2] [RESEND] wifi: carl9170: re-fix fortified-memset warning
Arnd Bergmann wrote:
> The carl9170_tx_release() function sometimes triggers a fortified-memset
> warning in my randconfig builds:
>
> In file included from include/linux/string.h:254,
> from drivers/net/wireless/ath/carl9170/tx.c:40:
> In function 'fortify_memset_chk',
> inlined from 'carl9170_tx_release' at
> drivers/net/wireless/ath/carl9170/tx.c:283:2,
> inlined from 'kref_put' at include/linux/kref.h:65:3,
> inlined from 'carl9170_tx_put_skb' at
> drivers/net/wireless/ath/carl9170/tx.c:342:9:
> include/linux/fortify-string.h:493:25: error: call to
> '__write_overflow_field' declared with attribute warning: detected write
> beyond size of field (1st parameter); maybe use struct_group()?
> [-Werror=attribute-warning]
> 493 | __write_overflow_field(p_size_field, size);
>
> Kees previously tried to avoid this by using memset_after(), but it seems
> this does not fully address the problem. I noticed that the memset_after()
> here is done on a different part of the union (status) than the original
> cast was from (rate_driver_data), which may confuse the compiler.
>
> Unfortunately, the memset_after() trick does not work on driver_rates[]
> because that is part of an anonymous struct, and I could not get
> struct_group() to do this either. Using two separate memset() calls
> on the two members does address the warning though.
>
> Fixes: fb5f6a0e8063b ("mac80211: Use memset_after() to clear tx status")
> Link: https://lore.kernel.org/lkml/20230623152443.2296825-1-a...@kernel.org/
> Signed-off-by: Arnd Bergmann
> Reviewed-by: Kees Cook
> Acked-by: Christian Lamparter
> Signed-off-by: Kalle Valo
2 patches applied to ath-next branch of ath.git, thanks.
066afafc10c9 wifi: carl9170: re-fix fortified-memset warning
61752ac69b69 wifi: ath9k: work around memset overflow warning
--
https://patchwork.kernel.org/project/linux-wireless/patch/20240328135509.3755090-2-a...@kernel.org/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2][next] wifi: wil6210: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid some -Wfamnae warnings
"Gustavo A. R. Silva" wrote: > Prepare for the coming implementation by GCC and Clang of the > __counted_by attribute. Flexible array members annotated with > __counted_by can have their accesses bounds-checked at run-time > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE > (for strcpy/memcpy-family functions). > > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are > getting ready to enable it globally. > > So, use the `DEFINE_FLEX()` helper for an on-stack definition of > a flexible structure where the size of the flexible-array member > is known at compile-time, and refactor the rest of the code, > accordingly. > > So, with these changes, fix the following warning: > drivers/net/wireless/ath/wil6210/cfg80211.c:896:43: warning: structure > containing a flexible array member is not at the end of another structure > [-Wflex-array-member-not-at-end] > > Link: https://github.com/KSPP/linux/issues/202 > Signed-off-by: Gustavo A. R. Silva > Reviewed-by: Jeff Johnson > Signed-off-by: Kalle Valo Patch applied to ath-next branch of ath.git, thanks. 34c34c242a1b wifi: wil6210: cfg80211: Use __counted_by() in struct wmi_start_scan_cmd and avoid some -Wfamnae warnings -- https://patchwork.kernel.org/project/linux-wireless/patch/ZgSP/CMSVfr68R2u@neat/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning
"Gustavo A. R. Silva" wrote: > Prepare for the coming implementation by GCC and Clang of the > __counted_by attribute. Flexible array members annotated with > __counted_by can have their accesses bounds-checked at run-time > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE > (for strcpy/memcpy-family functions). > > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are > getting ready to enable it globally. > > So, use the `DEFINE_FLEX()` helper for an on-stack definition of > a flexible structure where the size of the flexible-array member > is known at compile-time, and refactor the rest of the code, > accordingly. > > So, with these changes, fix the following warning: > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure containing > a flexible array member is not at the end of another structure > [-Wflex-array-member-not-at-end] > > Link: https://github.com/KSPP/linux/issues/202 > Signed-off-by: Gustavo A. R. Silva > Reviewed-by: Jeff Johnson > Signed-off-by: Kalle Valo Patch applied to ath-next branch of ath.git, thanks. cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning -- https://patchwork.kernel.org/project/linux-wireless/patch/ZgSTCmdP+omePvWg@neat/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
[PATCH v5][next] integrity: Avoid -Wflex-array-member-not-at-end warnings
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
There is currently an object (`hdr)` in `struct ima_max_digest_data`
that contains a flexible structure (`struct ima_digest_data`):
struct ima_max_digest_data {
struct ima_digest_data hdr;
u8 digest[HASH_MAX_DIGESTSIZE];
} __packed;
So, in order to avoid ending up with a flexible-array member in the
middle of a struct, we use the `__struct_group()` helper to separate
the flexible array from the rest of the members in the flexible
structure:
struct ima_digest_data {
__struct_group(ima_digest_data_hdr, hdr, __packed,
... the rest of the members
);
u8 digest[];
} __packed;
And similarly for `struct evm_ima_xattr_data`.
With the change described above, we can now declare an object of the
type of the tagged `struct ima_digest_data_hdr`, without embedding the
flexible array in the middle of another struct:
struct ima_max_digest_data {
struct ima_digest_data_hdr hdr;
u8 digest[HASH_MAX_DIGESTSIZE];
} __packed;
And similarly for `struct evm_digest` and `struct evm_xattr`.
We also use `container_of()` whenever we need to retrieve a pointer to
the flexible structure.
So, with these changes, fix the following warnings:
security/integrity/evm/evm.h:64:32: warning: structure containing a flexible
array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/evm/../integrity.h:40:35: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/evm/../integrity.h:68:32: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/ima/../integrity.h:40:35: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/ima/../integrity.h:68:32: warning: structure containing a
flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/integrity.h:40:35: warning: structure containing a flexible
array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/integrity.h:68:32: warning: structure containing a flexible
array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/platform_certs/../integrity.h:40:35: warning: structure
containing a flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
security/integrity/platform_certs/../integrity.h:68:32: warning: structure
containing a flexible array member is not at the end of another structure
[-Wflex-array-member-not-at-end]
Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva
---
Changes in v5:
- Rebase patch.
Changes in v4:
- Update changelog text. (Mimi Zohar)
- Include changes for `struct evm_xattr`.
- Add a couple of code comments.
- Link: https://lore.kernel.org/linux-hardening/ZgsAFhl90kecIR00@neat/
Changes in v3:
- struct ima_digest_data is a packed structure. So, to keep things
consistent, use the attribute __packed on the tagged struct
ima_digest_data_hdr. For this, we use __struct_group() instead of
struct_group_tagged(). Update the changelog text, accordingly.
- Link: https://lore.kernel.org/linux-hardening/ZfuzWku+ip4fsZrb@neat/
Changes in v2:
- Include changes for `struct evm_digest` (Mimi Zohar)
- Link: https://lore.kernel.org/linux-hardening/ZfuvoIj+AJHjCdTs@neat/
v1:
- Link: https://lore.kernel.org/linux-hardening/ZeYKWrXvACBBrAP8@neat/
security/integrity/evm/evm.h | 2 +-
security/integrity/ima/ima_api.c | 6 --
security/integrity/ima/ima_appraise.c | 4 +++-
security/integrity/ima/ima_init.c | 6 --
security/integrity/ima/ima_main.c | 6 --
security/integrity/ima/ima_template_lib.c | 10 ++
security/integrity/integrity.h| 12 +---
7 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index eb1a2c343bd7..72e3341ae6f7 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -61,7 +61,7 @@ extern int evm_hmac_attrs;
extern struct list_head evm_config_xattrnames;
struct evm_digest {
- struct ima_digest_data hdr;
+ struct ima_digest_data_hdr hdr;
char digest[IMA_MAX_DIGEST_SIZE];
} __packed;
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 1856981e33df..3d286de231e1 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -246,6 +246,8 @@ int ima_collect_measurement(struct ima_iint_cache *iint,
struct file *file,
struct inode *inode = file_inode(file);
struct ino
Re: [PATCH] fs: Set file_handle::handle_bytes before referencing file_handle::f_handle
On Thu, Apr 04, 2024 at 11:19:00AM +0200, Jan Kara wrote:
> On Wed 03-04-24 14:54:03, Kees Cook wrote:
> > With adding __counted_by(handle_bytes) to struct file_handle, we need
> > to explicitly set it in the one place it wasn't yet happening prior to
> > accessing the flex array "f_handle".
> >
> > Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by()
> > and use struct_size()")
> > Signed-off-by: Kees Cook
>
> OK, so this isn't really a functional bug AFAIU but the compiler will
> wrongly complain we are accessing handle->f_handle beyond claimed array
> size (because handle->handle_bytes == 0 at that point). Am I right? If
> that's the case, please add a short comment explaining this (because it
> looks odd we set handle->handle_bytes and then reset it a few lines later).
> With the comment feel free to add:
>
> Reviewed-by: Jan Kara
>
> Honza
I agree, an in-code comment is needed.
Acked-by: Chuck Lever
> > ---
> > Cc: Christian Brauner
> > Cc: "Gustavo A. R. Silva"
> > Cc: Alexander Viro
> > Cc: Jan Kara
> > Cc: Chuck Lever
> > Cc: Jeff Layton
> > Cc: Amir Goldstein
> > Cc: linux-fsde...@vger.kernel.org
> > Cc: linux-...@vger.kernel.org
> > Cc: linux-hardening@vger.kernel.org
> > ---
> > fs/fhandle.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 53ed54711cd2..08ec2340dd22 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
> > GFP_KERNEL);
> > if (!handle)
> > return -ENOMEM;
> > + handle->handle_bytes = f_handle.handle_bytes;
> >
> > /* convert handle size to multiple of sizeof(u32) */
> > handle_dwords = f_handle.handle_bytes >> 2;
> > --
> > 2.34.1
> >
> --
> Jan Kara
> SUSE Labs, CR
--
Chuck Lever
Re: [PATCH v5][next] integrity: Avoid -Wflex-array-member-not-at-end warnings
Hi Gustavo,
On Thu, 2024-04-04 at 09:00 -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
>
> There is currently an object (`hdr)` in `struct ima_max_digest_data`
> that contains a flexible structure (`struct ima_digest_data`):
>
> struct ima_max_digest_data {
> struct ima_digest_data hdr;
> u8 digest[HASH_MAX_DIGESTSIZE];
> } __packed;
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of a struct, we use the `__struct_group()` helper to separate
> the flexible array from the rest of the members in the flexible
> structure:
>
> struct ima_digest_data {
> __struct_group(ima_digest_data_hdr, hdr, __packed,
>
> ... the rest of the members
>
> );
> u8 digest[];
> } __packed;
>
> And similarly for `struct evm_ima_xattr_data`.
>
> With the change described above, we can now declare an object of the
> type of the tagged `struct ima_digest_data_hdr`, without embedding the
> flexible array in the middle of another struct:
>
> struct ima_max_digest_data {
> struct ima_digest_data_hdr hdr;
> u8 digest[HASH_MAX_DIGESTSIZE];
> } __packed;
>
> And similarly for `struct evm_digest` and `struct evm_xattr`.
>
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure.
>
> So, with these changes, fix the following warnings:
>
> security/integrity/evm/evm.h:64:32: warning: structure containing a flexible
> array member is not at the end of another structure [-Wflex-array-member-not-
> at-end]
> security/integrity/evm/../integrity.h:40:35: warning: structure containing a
> flexible array member is not at the end of another structure [-Wflex-array-
> member-not-at-end]
> security/integrity/evm/../integrity.h:68:32: warning: structure containing a
> flexible array member is not at the end of another structure [-Wflex-array-
> member-not-at-end]
> security/integrity/ima/../integrity.h:40:35: warning: structure containing a
> flexible array member is not at the end of another structure [-Wflex-array-
> member-not-at-end]
> security/integrity/ima/../integrity.h:68:32: warning: structure containing a
> flexible array member is not at the end of another structure [-Wflex-array-
> member-not-at-end]
> security/integrity/integrity.h:40:35: warning: structure containing a flexible
> array member is not at the end of another structure [-Wflex-array-member-not-
> at-end]
> security/integrity/integrity.h:68:32: warning: structure containing a flexible
> array member is not at the end of another structure [-Wflex-array-member-not-
> at-end]
> security/integrity/platform_certs/../integrity.h:40:35: warning: structure
> containing a flexible array member is not at the end of another structure [-
> Wflex-array-member-not-at-end]
> security/integrity/platform_certs/../integrity.h:68:32: warning: structure
> containing a flexible array member is not at the end of another structure [-
> Wflex-array-member-not-at-end]
>
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Mimi Zohar
Thanks, Gustavo. I already applied and tested v4, but was trying to actually
see the errors with before pushing it out.
Mimi
Re: [PATCH 7/7] arm64: dts: qcom: Add SM8550 Xperia 1 V
On 2/12/24 18:26, Neil Armstrong wrote: On 12/02/2024 14:10, Konrad Dybcio wrote: Add support for Sony Xperia 1 V, a.k.a PDX234. This device is a part of the SoMC SM8550 Yodo platform. [...] +/* TODO: Only one SID of PMR735D seems accessible? */ What's reported by the cpuinfo pmic array ? PMK8550 2.1 PM8550 2.0 PM8550VS 2.0 PM8550VS 2.0 PM8550VS 2.0 PM8550VE 2.0 PM8550VS 2.0 PM8550B 2.0 PMR735D 2.0 PM8010 1.1 PM8010 1.1 Not sure if there's only one or the other one is secure? With the pcie thing fixed: Reviewed-by: Neil Armstrong It's gonna be fine with the recent aux clock additions. If you have no further comments, I'll happily ask for this to be merged ;) Konrad
Re: [PATCH v5][next] integrity: Avoid -Wflex-array-member-not-at-end warnings
On Thu, 2024-04-04 at 12:49 -0400, Mimi Zohar wrote:
> Hi Gustavo,
>
> On Thu, 2024-04-04 at 09:00 -0600, Gustavo A. R. Silva wrote:
> > -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> > ready to enable it globally.
> >
> > There is currently an object (`hdr)` in `struct ima_max_digest_data`
> > that contains a flexible structure (`struct ima_digest_data`):
> >
> > struct ima_max_digest_data {
> > struct ima_digest_data hdr;
> > u8 digest[HASH_MAX_DIGESTSIZE];
> > } __packed;
> >
> > So, in order to avoid ending up with a flexible-array member in the
> > middle of a struct, we use the `__struct_group()` helper to separate
> > the flexible array from the rest of the members in the flexible
> > structure:
> >
> > struct ima_digest_data {
> > __struct_group(ima_digest_data_hdr, hdr, __packed,
> >
> > ... the rest of the members
> >
> > );
> > u8 digest[];
> > } __packed;
> >
> > And similarly for `struct evm_ima_xattr_data`.
> >
> > With the change described above, we can now declare an object of the
> > type of the tagged `struct ima_digest_data_hdr`, without embedding the
> > flexible array in the middle of another struct:
> >
> > struct ima_max_digest_data {
> > struct ima_digest_data_hdr hdr;
> > u8 digest[HASH_MAX_DIGESTSIZE];
> > } __packed;
> >
> > And similarly for `struct evm_digest` and `struct evm_xattr`.
> >
> > We also use `container_of()` whenever we need to retrieve a pointer to
> > the flexible structure.
> >
> > So, with these changes, fix the following warnings:
> >
> > security/integrity/evm/evm.h:64:32: warning: structure containing a flexible
> > array member is not at the end of another structure [-Wflex-array-member-
> > not-
> > at-end]
> > security/integrity/evm/../integrity.h:40:35: warning: structure containing a
> > flexible array member is not at the end of another structure [-Wflex-array-
> > member-not-at-end]
> > security/integrity/evm/../integrity.h:68:32: warning: structure containing a
> > flexible array member is not at the end of another structure [-Wflex-array-
> > member-not-at-end]
> > security/integrity/ima/../integrity.h:40:35: warning: structure containing a
> > flexible array member is not at the end of another structure [-Wflex-array-
> > member-not-at-end]
> > security/integrity/ima/../integrity.h:68:32: warning: structure containing a
> > flexible array member is not at the end of another structure [-Wflex-array-
> > member-not-at-end]
> > security/integrity/integrity.h:40:35: warning: structure containing a
> > flexible
> > array member is not at the end of another structure [-Wflex-array-member-
> > not-
> > at-end]
> > security/integrity/integrity.h:68:32: warning: structure containing a
> > flexible
> > array member is not at the end of another structure [-Wflex-array-member-
> > not-
> > at-end]
> > security/integrity/platform_certs/../integrity.h:40:35: warning: structure
> > containing a flexible array member is not at the end of another structure [-
> > Wflex-array-member-not-at-end]
> > security/integrity/platform_certs/../integrity.h:68:32: warning: structure
> > containing a flexible array member is not at the end of another structure [-
> > Wflex-array-member-not-at-end]
> >
> > Link: https://github.com/KSPP/linux/issues/202
> > Signed-off-by: Gustavo A. R. Silva
>
> Reviewed-by: Mimi Zohar
>
> Thanks, Gustavo. I already applied and tested v4, but was trying to actually
> see the errors with before pushing it out.
With GCC-14, I'm seeing the "warning: structure containing a flexible array
member is not at the end of another structure [-Wflex-array-member-not-at-end]"
messages. As expected, with the patch no warnings.
"checkpatch.pl --strict" complains "CHECK: Alignment should match open
parenthesis". I'll queue the patch, but how about teaching checkpatch.pl to
ignore __struct_group()?
thanks,
Mimi
Re: [PATCH] init: replace deprecated strncpy with strscpy_pad
On Tue, Apr 02, 2024 at 08:39:49PM +, 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. > > data_page wants to be NUL-terminated and NUL-padded, use strscpy_pad to > provide both of these. data_page no longer awkwardly relies on > init_mount to perform its NUL-termination, although that sanity check is > left unchanged. > > 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 This looks good. Thanks! Reviewed-by: Kees Cook -- Kees Cook
[PATCH v2] fs: Set file_handle::handle_bytes before referencing file_handle::f_handle
Since __counted_by(handle_bytes) was added to struct file_handle, we need
to explicitly set it in the one place it wasn't yet happening prior to
accessing the flex array "f_handle". For robustness also check for a
negative value for handle_bytes, which is possible for an "int", but
nothing appears to set.
Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and
use struct_size()")
Signed-off-by: Kees Cook
---
Cc: Jan Kara
Cc: Chuck Lever
Cc: Gustavo A. R. Silva
Cc: Christian Brauner
Cc: Alexander Viro
Cc: Jeff Layton
Cc: Amir Goldstein
Cc: linux-fsde...@vger.kernel.org
Cc: linux-...@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
v2: more bounds checking, add comments, dropped reviews since logic changed
v1: https://lore.kernel.org/all/20240403215358.work.365-k...@kernel.org/
---
fs/fhandle.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8a7f86c2139a..854f866eaad2 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -40,6 +40,11 @@ static long do_sys_name_to_handle(const struct path *path,
GFP_KERNEL);
if (!handle)
return -ENOMEM;
+ /*
+* Since handle->f_handle is about to be written, make sure the
+* associated __counted_by(handle_bytes) variable is correct.
+*/
+ handle->handle_bytes = f_handle.handle_bytes;
/* convert handle size to multiple of sizeof(u32) */
handle_dwords = f_handle.handle_bytes >> 2;
@@ -51,8 +56,8 @@ static long do_sys_name_to_handle(const struct path *path,
handle->handle_type = retval;
/* convert handle size to bytes */
handle_bytes = handle_dwords * sizeof(u32);
- handle->handle_bytes = handle_bytes;
- if ((handle->handle_bytes > f_handle.handle_bytes) ||
+ /* check if handle_bytes would have exceeded the allocation */
+ if ((handle_bytes < 0) || (handle_bytes > f_handle.handle_bytes) ||
(retval == FILEID_INVALID) || (retval < 0)) {
/* As per old exportfs_encode_fh documentation
* we could return ENOSPC to indicate overflow
@@ -68,6 +73,8 @@ static long do_sys_name_to_handle(const struct path *path,
handle_bytes = 0;
} else
retval = 0;
+ /* the "valid" number of bytes may fewer than originally allocated */
+ handle->handle_bytes = handle_bytes;
/* copy the mount id */
if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
copy_to_user(ufh, handle,
--
2.34.1
Re: [PATCH v2] hfsplus: refactor copy_name to not use strncpy
On Mon, Apr 01, 2024 at 06:10:48PM +, Justin Stitt wrote:
> strncpy() is deprecated with NUL-terminated destination strings [1].
>
> The copy_name() method does a lot of manual buffer manipulation to
> eventually arrive with its desired string. If we don't know the
> namespace this attr has or belongs to we want to prepend "osx." to our
> final string. Following this, we're copying xattr_name and doing a
> bizarre manual NUL-byte assignment with a memset where n=1.
>
> Really, we can use some more obvious string APIs to acomplish this,
> improving readability and security. Following the same control flow as
> before: if we don't know the namespace let's use scnprintf() to form our
> prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use
> strscpy() to return the number of bytes copied into our buffer.
> Additionally, for non-empty strings, include the NUL-byte in the length
> -- matching the behavior of the previous implementation.
>
> Note that strscpy() _can_ return -E2BIG but this is already handled by
> all callsites:
>
> In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is
> already type ssize_t so we can change the return type of copy_name() to
> match (understanding that scnprintf()'s return type is different yet
> fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c
> is well-equipped to handle a potential -E2BIG return result from
> vfs_listxattr():
> | ssize_t error;
> ...
> | error = vfs_listxattr(d, klist, size);
> | if (error > 0) {
> | if (size && copy_to_user(list, klist, error))
> | error = -EFAULT;
> | } else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
> | /* The file system tried to returned a list bigger
> | than XATTR_LIST_MAX bytes. Not possible. */
> | error = -E2BIG;
> | }
> ... the error can potentially already be -E2BIG, skipping this else-if
> and ending up at the same state as other errors.
>
> 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
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt
Thanks, this looks right to me now!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v2] fs: Set file_handle::handle_bytes before referencing file_handle::f_handle
On Thu, Apr 04, 2024 at 02:12:15PM -0700, Kees Cook wrote:
> Since __counted_by(handle_bytes) was added to struct file_handle, we need
> to explicitly set it in the one place it wasn't yet happening prior to
> accessing the flex array "f_handle". For robustness also check for a
> negative value for handle_bytes, which is possible for an "int", but
> nothing appears to set.
Why not change handle_bytes from an int to a u32?
Also, what a grotty function.
handle_dwords = f_handle.handle_bytes >> 2;
...
handle_bytes = handle_dwords * sizeof(u32);
> Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and
> use struct_size()")
> Signed-off-by: Kees Cook
> ---
> Cc: Jan Kara
> Cc: Chuck Lever
> Cc: Gustavo A. R. Silva
> Cc: Christian Brauner
> Cc: Alexander Viro
> Cc: Jeff Layton
> Cc: Amir Goldstein
> Cc: linux-fsde...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> v2: more bounds checking, add comments, dropped reviews since logic changed
> v1: https://lore.kernel.org/all/20240403215358.work.365-k...@kernel.org/
> ---
> fs/fhandle.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8a7f86c2139a..854f866eaad2 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -40,6 +40,11 @@ static long do_sys_name_to_handle(const struct path *path,
>GFP_KERNEL);
> if (!handle)
> return -ENOMEM;
> + /*
> + * Since handle->f_handle is about to be written, make sure the
> + * associated __counted_by(handle_bytes) variable is correct.
> + */
> + handle->handle_bytes = f_handle.handle_bytes;
>
> /* convert handle size to multiple of sizeof(u32) */
> handle_dwords = f_handle.handle_bytes >> 2;
> @@ -51,8 +56,8 @@ static long do_sys_name_to_handle(const struct path *path,
> handle->handle_type = retval;
> /* convert handle size to bytes */
> handle_bytes = handle_dwords * sizeof(u32);
> - handle->handle_bytes = handle_bytes;
> - if ((handle->handle_bytes > f_handle.handle_bytes) ||
> + /* check if handle_bytes would have exceeded the allocation */
> + if ((handle_bytes < 0) || (handle_bytes > f_handle.handle_bytes) ||
> (retval == FILEID_INVALID) || (retval < 0)) {
> /* As per old exportfs_encode_fh documentation
>* we could return ENOSPC to indicate overflow
> @@ -68,6 +73,8 @@ static long do_sys_name_to_handle(const struct path *path,
> handle_bytes = 0;
> } else
> retval = 0;
> + /* the "valid" number of bytes may fewer than originally allocated */
> + handle->handle_bytes = handle_bytes;
> /* copy the mount id */
> if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
> copy_to_user(ufh, handle,
> --
> 2.34.1
>
>
Re: [PATCH v5][next] integrity: Avoid -Wflex-array-member-not-at-end warnings
"checkpatch.pl --strict" complains "CHECK: Alignment should match open
parenthesis". I'll queue the patch, but how about teaching checkpatch.pl to
ignore __struct_group()?
I think this would do it:
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9c4c4a61bc83..e229b97f17f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3958,7 +3958,7 @@ sub process {
my $rest = $2;
my $pos = pos_last_openparen($rest);
- if ($pos >= 0) {
+ if ($pos >= 0 && $rest !~
/(__)?struct_group(_(tagged|attr))?(\()/) {
$line =~ /^(\+| )([ \t]*)/;
my $newindent = $2;
I'll send a proper patch. Thanks for the suggestion, Mimi.
--
Gustavo
