After mounting a remote cifs resource, it becomes unavailable:
df: /mnt/sambashare: Resource temporarily unavailable
It was tested on the following Linux kernels:
Linux altlinux 5.10.208-std-def-alt1
Linux fedora 5.10.208-200.el8.x86_64
The error appeared starting from kernel 5.10.206 after adding
the commit [1] "smb: client: fix OOB in SMB2_query_info_init()",
in which the buffer length increases by 1 as a result of changes:
..
- iov[0].iov_len = total_len - 1 + input_len;
+ iov[0].iov_len = len;
..
[1] https://patchwork.kernel.org/project/cifs-client/patch/[email protected]/
Error fixed by backported commits in next two patches adapted for the 5.10 kernel:
[PATCH 1/2] stddef: Introduce DECLARE_FLEX_ARRAY() helper
[PATCH 2/2] smb3: Replace smb2pdu 1-element arrays with flex-arrays
From: Kees Cook <[email protected]>
commit 3080ea5553cc909b000d1f1d964a9041962f2c5b upstream.
There are many places where kernel code wants to have several different
typed trailing flexible arrays. This would normally be done with multiple
flexible arrays in a union, but since GCC and Clang don't (on the surface)
allow this, there have been many open-coded workarounds, usually involving
neighboring 0-element arrays at the end of a structure. For example,
instead of something like this:
struct thing {
...
union {
struct type1 foo[];
struct type2 bar[];
};
};
code works around the compiler with:
struct thing {
...
struct type1 foo[0];
struct type2 bar[];
};
Another case is when a flexible array is wanted as the single member
within a struct (which itself is usually in a union). For example, this
would be worked around as:
union many {
...
struct {
struct type3 baz[0];
};
};
These kinds of work-arounds cause problems with size checks against such
zero-element arrays (for example when building with -Warray-bounds and
-Wzero-length-bounds, and with the coming FORTIFY_SOURCE improvements),
so they must all be converted to "real" flexible arrays, avoiding warnings
like this:
fs/hpfs/anode.c: In function 'hpfs_add_sector_to_btree':
fs/hpfs/anode.c:209:27: warning: array subscript 0 is outside the bounds of an interior zero-length array 'struct bplus_internal_node[0]' [-Wzero-length-bounds]
209 | anode->btree.u.internal[0].down = cpu_to_le32(a);
| ~~~~~~~~~~~~~~~~~~~~~~~^~~
In file included from fs/hpfs/hpfs_fn.h:26,
from fs/hpfs/anode.c:10:
fs/hpfs/hpfs.h:412:32: note: while referencing 'internal'
412 | struct bplus_internal_node internal[0]; /* (internal) 2-word entries giving
| ^~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_fd.c: In function 'es58x_fd_tx_can_msg':
drivers/net/can/usb/etas_es58x/es58x_fd.c:360:35: warning: array subscript 65535 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[]'} [-Wzero-length-bounds]
360 | tx_can_msg = (typeof(tx_can_msg))&es58x_fd_urb_cmd->raw_msg[msg_len];
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/can/usb/etas_es58x/es58x_core.h:22,
from drivers/net/can/usb/etas_es58x/es58x_fd.c:17:
drivers/net/can/usb/etas_es58x/es58x_fd.h:231:6: note: while referencing 'raw_msg'
231 | u8 raw_msg[0];
| ^~~~~~~
However, it _is_ entirely possible to have one or more flexible arrays
in a struct or union: it just has to be in another struct. And since it
cannot be alone in a struct, such a struct must have at least 1 other
named member -- but that member can be zero sized. Wrap all this nonsense
into the new DECLARE_FLEX_ARRAY() in support of having flexible arrays
in unions (or alone in a struct).
As with struct_group(), since this is needed in UAPI headers as well,
implement the core there, with a non-UAPI wrapper.
Additionally update kernel-doc to understand its existence.
https://github.com/KSPP/linux/issues/137
Cc: Arnd Bergmann <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
Signed-off-by: Vasiliy Kovalev <[email protected]>
---
include/linux/stddef.h | 13 +++++++++++++
include/uapi/linux/stddef.h | 16 ++++++++++++++++
scripts/kernel-doc | 3 ++-
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 938216f8ab7e7c..31fdbb784c24e2 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -84,4 +84,17 @@ enum {
#define struct_group_tagged(TAG, NAME, MEMBERS...) \
__struct_group(TAG, NAME, /* no attrs */, MEMBERS)
+/**
+ * DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
+ *
+ * @TYPE: The type of each flexible array element
+ * @NAME: The name of the flexible array member
+ *
+ * In order to have a flexible array member in a union or alone in a
+ * struct, it needs to be wrapped in an anonymous struct with at least 1
+ * named member, but that member can be empty.
+ */
+#define DECLARE_FLEX_ARRAY(TYPE, NAME) \
+ __DECLARE_FLEX_ARRAY(TYPE, NAME)
+
#endif
diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
index c3725b49226323..7837ba4fe72890 100644
--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -28,4 +28,20 @@
struct { MEMBERS } ATTRS; \
struct TAG { MEMBERS } ATTRS NAME; \
}
+
+/**
+ * __DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
+ *
+ * @TYPE: The type of each flexible array element
+ * @NAME: The name of the flexible array member
+ *
+ * In order to have a flexible array member in a union or alone in a
+ * struct, it needs to be wrapped in an anonymous struct with at least 1
+ * named member, but that member can be empty.
+ */
+#define __DECLARE_FLEX_ARRAY(TYPE, NAME) \
+ struct { \
+ struct { } __empty_ ## NAME; \
+ TYPE NAME[]; \
+ }
#endif
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 19af6dd160e6b7..7a04d4c0532607 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1232,7 +1232,8 @@ sub dump_struct($$) {
$members =~ s/DECLARE_KFIFO\s*\(([^,)]+),\s*([^,)]+),\s*([^,)]+)\)/$2 \*$1/gos;
# replace DECLARE_KFIFO_PTR
$members =~ s/DECLARE_KFIFO_PTR\s*\(([^,)]+),\s*([^,)]+)\)/$2 \*$1/gos;
-
+ # replace DECLARE_FLEX_ARRAY
+ $members =~ s/(?:__)?DECLARE_FLEX_ARRAY\s*\($args,\s*$args\)/$1 $2\[\]/gos;
my $declaration = $members;
# Split nested struct/union elements as newer ones
--
2.33.8
On Fri, Jan 26, 2024 at 10:31:41PM +0300, [email protected] wrote:
> After mounting a remote cifs resource, it becomes unavailable:
> df: /mnt/sambashare: Resource temporarily unavailable
>
> It was tested on the following Linux kernels:
> Linux altlinux 5.10.208-std-def-alt1
> Linux fedora 5.10.208-200.el8.x86_64
>
> The error appeared starting from kernel 5.10.206 after adding
> the commit [1] "smb: client: fix OOB in SMB2_query_info_init()",
> in which the buffer length increases by 1 as a result of changes:
> ...
> - iov[0].iov_len = total_len - 1 + input_len;
> + iov[0].iov_len = len;
> ...
>
> [1] https://patchwork.kernel.org/project/cifs-client/patch/[email protected]/
>
> Error fixed by backported commits in next two patches adapted for the 5.10 kernel:
>
> [PATCH 1/2] stddef: Introduce DECLARE_FLEX_ARRAY() helper
> [PATCH 2/2] smb3: Replace smb2pdu 1-element arrays with flex-arrays
>
>
Now queued up, thanks.
greg k-h
Hi,
Adding more people to CC.(who have looked at this issue)
On 27/01/24 6:19 am, Greg KH wrote:
> On Fri, Jan 26, 2024 at 10:31:41PM +0300, [email protected] wrote:
>> After mounting a remote cifs resource, it becomes unavailable:
>> df: /mnt/sambashare: Resource temporarily unavailable
>>
>> It was tested on the following Linux kernels:
>> Linux altlinux 5.10.208-std-def-alt1
>> Linux fedora 5.10.208-200.el8.x86_64
>>
>> The error appeared starting from kernel 5.10.206 after adding
>> the commit [1] "smb: client: fix OOB in SMB2_query_info_init()",
>> in which the buffer length increases by 1 as a result of changes:
>> ...
>> - iov[0].iov_len = total_len - 1 + input_len;
>> + iov[0].iov_len = len;
>> ...
>>
We can reproduce this on 5.15.148(latest 5.15.y) and Mohamed reported
this on 6.1.y, so we need backports there as well.
https://lore.kernel.org/all/[email protected]/
[root@vm1 xfstests-dev]# ./check -g quick -s smb3
TEST_DEV=//<SERVER_IP>/TEST is mounted but not a type cifs filesystem
[root@vm1 xfstests-dev]# df
df: /mnt/test: Resource temporarily unavailable
This two patch series doesn't cleanly apply to 5.15.y.
Also I am unsure, which is the better approach to go with
Approach 1 - suggested by Paulo:
https://lore.kernel.org/all/[email protected]/
Approach 2 - this series
Pulling in [PATCH 2/2] smb3: Replace smb2pdu 1-element arrays with
flex-arrays like this series did.
I think approach 1 is better as the changes are minimal, but please
correct me if that seems wrong.
Thanks,
Harshit
>> [1] https://patchwork.kernel.org/project/cifs-client/patch/[email protected]/
>>
>> Error fixed by backported commits in next two patches adapted for the 5.10 kernel:
>>
>> [PATCH 1/2] stddef: Introduce DECLARE_FLEX_ARRAY() helper
>> [PATCH 2/2] smb3: Replace smb2pdu 1-element arrays with flex-arrays
>>
>>
>
> Now queued up, thanks.
>
> greg k-h
>
Hi,
27.01.2024 09:42, Harshit Mogalapalli wrote:
> We can reproduce this on 5.15.148(latest 5.15.y) and Mohamed reported
> this on 6.1.y, so we need backports there as well.
in the 6.1.72 kernel, this problem was fixed by the commit [1] "smb3:
Replace smb2pdu 1-element arrays with flex-arrays", which was proposed
in this series of patches.
[1] https://lore.kernel.org/all/2024010937-eggplant-bauble-d556@gregkh/T/
--
Regards,
Vasiliy Kovalev
Hi Kovalev,
On 27/01/24 1:32 pm, [email protected] wrote:
> Hi,
>
> 27.01.2024 09:42, Harshit Mogalapalli wrote:
>> We can reproduce this on 5.15.148(latest 5.15.y) and Mohamed reported
>> this on 6.1.y, so we need backports there as well.
>
> in the 6.1.72 kernel, this problem was fixed by the commit [1] "smb3:
> Replace smb2pdu 1-element arrays with flex-arrays", which was proposed
> in this series of patches.
>
Thanks for sharing this, I didnot notice that the above commit was
backported to 6.1.72.
I think we still need fixing in 5.15.y as the commit eb3e28c1e89b
("smb3: Replace smb2pdu 1-element arrays with flex-arrays") is not in
5.15.148
Thanks,
Harshit
>
> [1] https://lore.kernel.org/all/2024010937-eggplant-bauble-d556@gregkh/T/
>
On Sat, Jan 27, 2024 at 06:59:15PM +0530, Harshit Mogalapalli wrote:
> Hi Kovalev,
>
> On 27/01/24 1:32 pm, [email protected] wrote:
> > Hi,
> >
> > 27.01.2024 09:42, Harshit Mogalapalli wrote:
> > > We can reproduce this on 5.15.148(latest 5.15.y) and Mohamed
> > > reported this on 6.1.y, so we need backports there as well.
> >
> > in the 6.1.72 kernel, this problem was fixed by the commit [1] "smb3:
> > Replace smb2pdu 1-element arrays with flex-arrays", which was proposed
> > in this series of patches.
> >
> Thanks for sharing this, I didnot notice that the above commit was
> backported to 6.1.72.
>
> I think we still need fixing in 5.15.y as the commit eb3e28c1e89b ("smb3:
> Replace smb2pdu 1-element arrays with flex-arrays") is not in 5.15.148
Patches gladly accepted :)
On Sat, Jan 27, 2024 at 12:43 AM Harshit Mogalapalli
<[email protected]> wrote:
>
> Hi,
>
> Adding more people to CC.(who have looked at this issue)
>
> On 27/01/24 6:19 am, Greg KH wrote:
> > On Fri, Jan 26, 2024 at 10:31:41PM +0300, [email protected] wrote:
> >> After mounting a remote cifs resource, it becomes unavailable:
> >> df: /mnt/sambashare: Resource temporarily unavailable
> >>
> >> It was tested on the following Linux kernels:
> >> Linux altlinux 5.10.208-std-def-alt1
> >> Linux fedora 5.10.208-200.el8.x86_64
> >>
> >> The error appeared starting from kernel 5.10.206 after adding
> >> the commit [1] "smb: client: fix OOB in SMB2_query_info_init()",
> >> in which the buffer length increases by 1 as a result of changes:
> >> ...
> >> - iov[0].iov_len = total_len - 1 + input_len;
> >> + iov[0].iov_len = len;
> >> ...
> >>
>
> We can reproduce this on 5.15.148(latest 5.15.y) and Mohamed reported
> this on 6.1.y, so we need backports there as well.
>
> https://lore.kernel.org/all/[email protected]/
>
>
> [root@vm1 xfstests-dev]# ./check -g quick -s smb3
> TEST_DEV=//<SERVER_IP>/TEST is mounted but not a type cifs filesystem
> [root@vm1 xfstests-dev]# df
> df: /mnt/test: Resource temporarily unavailable
>
>
> This two patch series doesn't cleanly apply to 5.15.y.
>
> Also I am unsure, which is the better approach to go with
>
> Approach 1 - suggested by Paulo:
> https://lore.kernel.org/all/[email protected]/
>
> Approach 2 - this series
> Pulling in [PATCH 2/2] smb3: Replace smb2pdu 1-element arrays with
> flex-arrays like this series did.
>
> I think approach 1 is better as the changes are minimal, but please
> correct me if that seems wrong.
Yes - Paulo's fix looks simple
--
Thanks,
Steve