Re: [PATCH] mxser: Annotate struct mxser_board with __counted_by
On 22. 09. 23, 19:52, Kees Cook 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 checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct mxser_board. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Reviewed-by: Jiri Slaby thanks, -- js suse labs
Re: [PATCH] ath5k: replace deprecated strncpy with strscpy
On 13. 10. 23, 22:53, 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.
We expect led->name to be NUL-terminated based on the presence of a
manual NUL-byte assignment.
This NUL-byte assignment was added in Commit daf9669bea30aa22 ("ath5k:
ensure led name is null terminated"). If strscpy() had existed and had
been used back when this code was written then potential bugs and the
need to manually NUL-terminate could have been avoided. Since we now
have the technology, let's use it :)
Considering the above, a suitable replacement is `strscpy` [2] due to
the fact that it guarantees NUL-termination on the destination buffer
without unnecessarily NUL-padding. If NUL-padding is required let's opt
for strscpy_pad().
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
LGTM
Reviewed-by: Jiri Slaby
--
js
suse labs
Re: [PATCH v2] tty: rfcomm: prefer struct_size over open coded arithmetic
On 12. 05. 24, 13:17, 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 "dl" variable is a pointer to "struct rfcomm_dev_list_req" and
this structure ends in a flexible array:
...
--- a/include/net/bluetooth/rfcomm.h
+++ b/include/net/bluetooth/rfcomm.h
...
@@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg)
list_for_each_entry(dev, &rfcomm_dev_list, list) {
if (!tty_port_get(&dev->port))
continue;
- (di + n)->id = dev->id;
- (di + n)->flags = dev->flags;
- (di + n)->state = dev->dlc->state;
- (di + n)->channel = dev->channel;
- bacpy(&(di + n)->src, &dev->src);
- bacpy(&(di + n)->dst, &dev->dst);
+ di[n].id = dev->id;
+ di[n].flags = dev->flags;
+ di[n].state = dev->dlc->state;
+ di[n].channel = dev->channel;
+ bacpy(&di[n].src, &dev->src);
+ bacpy(&di[n].dst, &dev->dst);
This does not relate much to "prefer struct_size over open coded
arithmetic". It should have been in a separate patch.
Other than that, LGTM.
thanks,
--
js
suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 29. 05. 24, 23:29, Nathan Chancellor wrote:
Work for __counted_by on generic pointers in structures (not just
flexible array members) has started landing in Clang 19 (current tip of
tree). During the development of this feature, a restriction was added
to __counted_by to prevent the flexible array member's element type from
including a flexible array member itself such as:
struct foo {
int count;
char buf[];
};
struct bar {
int count;
struct foo data[] __counted_by(count);
};
because the size of data cannot be calculated with the standard array
size formula:
sizeof(struct foo) * count
This restriction was downgraded to a warning but due to CONFIG_WERROR,
it can still break the build. The application of __counted_by on the
ports member of 'struct mxser_board' triggers this restriction,
resulting in:
drivers/tty/mxser.c:291:2: error: 'counted_by' should not be applied to an
array with element of unknown size because 'struct mxser_port' is a struct type
with a flexible array member.
Huh -- what am I missing:
struct mxser_port {
struct tty_port port;
struct mxser_board *board;
unsigned long ioaddr;
unsigned long opmode_ioaddr;
u8 rx_high_water;
u8 rx_low_water;
int type; /* UART type */
u8 x_char; /* xon/xoff character */
u8 IER; /* Interrupt Enable Register */
u8 MCR; /* Modem control register */
u8 FCR; /* FIFO control register */
struct async_icount icount;
unsigned int timeout;
u8 read_status_mask;
u8 ignore_status_mask;
u8 xmit_fifo_size;
spinlock_t slock;
};
?
This will be an error in a future compiler version
[-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
291 | struct mxser_port ports[] __counted_by(nports);
| ^
1 error generated.
Remove this use of __counted_by to fix the warning/error. However,
rather than remove it altogether, leave it commented, as it may be
possible to support this in future compiler releases.
This looks like a compiler bug/deficiency.
What does gcc say BTW?
Cc: sta...@vger.kernel.org
Closes: https://github.com/ClangBuiltLinux/linux/issues/2026
Fixes: f34907ecca71 ("mxser: Annotate struct mxser_board with __counted_by")
I would not say "Fixes" here. It only works around a broken compiler.
Signed-off-by: Nathan Chancellor
---
drivers/tty/mxser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 458bb1280ebf..5b97e420a95f 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -288,7 +288,7 @@ struct mxser_board {
enum mxser_must_hwid must_hwid;
speed_t max_baud;
- struct mxser_port ports[] __counted_by(nports);
+ struct mxser_port ports[] /* __counted_by(nports) */;
};
static DECLARE_BITMAP(mxser_boards, MXSER_BOARDS);
thanks,
--
js
suse labs
Re: [PATCH] nvmet-fc: Remove __counted_by from nvmet_fc_tgt_queue.fod[]
On 29. 05. 24, 23:42, Nathan Chancellor wrote:
drivers/nvme/target/fc.c:151:2: error: 'counted_by' should not be applied to
an array with element of unknown size because 'struct nvmet_fc_fcp_iod' is a
struct type with a flexible array member.
The same as for mxser_port:
struct nvmet_fc_fcp_iod {
struct nvmefc_tgt_fcp_req *fcpreq;
struct nvme_fc_cmd_iu cmdiubuf;
struct nvme_fc_ersp_iu rspiubuf;
dma_addr_t rspdma;
struct scatterlist *next_sg;
struct scatterlist *data_sg;
int data_sg_cnt;
u32 offset;
enum nvmet_fcp_datadir io_dir;
boolactive;
boolabort;
boolaborted;
boolwritedataactive;
spinlock_t flock;
struct nvmet_reqreq;
struct work_struct defer_work;
struct nvmet_fc_tgtport *tgtport;
struct nvmet_fc_tgt_queue *queue;
struct list_headfcp_list; /*
tgtport->fcp_list */
};
The error appears to be invalid.
This will be an error in a future compiler version
[-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size]
151 | struct nvmet_fc_fcp_iod fod[] __counted_by(sqsize);
| ^
1 error generated.
--
--
js
suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote: On 30/05/24 09:40, Greg Kroah-Hartman wrote: On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] 291 | struct mxser_port ports[] __counted_by(nports); | ^ 1 error generated. Remove this use of __counted_by to fix the warning/error. However, rather than remove it altogether, leave it commented, as it may be possible to support this in future compiler releases. This looks like a compiler bug/deficiency. I agree, why not just turn that option off in the compiler so that these "warnings" will not show up? It's not a compiler bug. It is, provided the code compiles and runs. The flexible array is nested four struct layers deep, see: ports[].port.buf.sentinel.data[] The error report could be more specific, though. Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it all works. The size is well known, [] is zero size, right? Still, fix the compiler, not the code. thanks, -- js suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30. 05. 24, 10:33, Jiri Slaby wrote: On 30. 05. 24, 10:12, Gustavo A. R. Silva wrote: On 30/05/24 09:40, Greg Kroah-Hartman wrote: On Thu, May 30, 2024 at 08:22:03AM +0200, Jiri Slaby wrote: This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] 291 | struct mxser_port ports[] __counted_by(nports); | ^ 1 error generated. Remove this use of __counted_by to fix the warning/error. However, rather than remove it altogether, leave it commented, as it may be possible to support this in future compiler releases. This looks like a compiler bug/deficiency. I agree, why not just turn that option off in the compiler so that these "warnings" will not show up? It's not a compiler bug. It is, provided the code compiles and runs. The flexible array is nested four struct layers deep, see: ports[].port.buf.sentinel.data[] The error report could be more specific, though. Ah, ok. The assumption is sentinel.data[] shall be unused. That's why it all works. The size is well known, [] is zero size, right? Still, fix the compiler, not the code. Or fix the code (properly). Flex arrays (even empty) in the middle of structs (like ports[].port.buf.sentinel.data[] above is) are deprecated since gcc 14: https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626516.html So we should get rid of all those. Sooner than later. thanks, -- -- js suse labs
Re: [PATCH] tty: mxser: Remove __counted_by from mxser_board.ports[]
On 30. 05. 24, 10:46, Gustavo A. R. Silva wrote: So we should get rid of all those. Sooner than later. Yes! Please do this. Definitely, we are working on that already[1]. :) FWIW undoing: commit 7391ee16950e772076d321792d9fbf030f921345 Author: Peter Hurley Date: Sat Jun 15 09:36:07 2013 -0400 tty: Simplify flip buffer list with 0-sized sentinel would do the job, IMO. thanks, -- js suse labs
Re: [PATCH] lib: string_helpers: fix potential snprintf() output truncation
On 21. 10. 24, 12:04, Bartosz Golaszewski wrote:
From: Bartosz Golaszewski
The output of ".%03u" with the unsigned int in range [0, 4294966295] may
get truncated if the target buffer is not 12 bytes.
Perhaps, if you elaborate on how 'remainder' can become > 999?
Fixes: 3c9f3681d0b4 ("[SCSI] lib: add generic helper to print sizes rounded to the
correct SI range")
Cc: sta...@vger.kernel.org
Reviewed-by: Andy Shevchenko
Signed-off-by: Bartosz Golaszewski
---
lib/string_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 4f887aa62fa0..91fa37b5c510 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -57,7 +57,7 @@ int string_get_size(u64 size, u64 blk_size, const enum
string_size_units units,
static const unsigned int rounding[] = { 500, 50, 5 };
int i = 0, j;
u32 remainder = 0, sf_cap;
- char tmp[8];
+ char tmp[12];
const char *unit;
tmp[0] = '\0';
--
js
suse labs
Re: [PATCH][next] tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings
On 05. 02. 25, 6:36, Greg Kroah-Hartman wrote: On Wed, Feb 05, 2025 at 03:51:35PM +1030, Gustavo A. R. Silva wrote: -Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. So, in order to avoid ending up with a flexible-array member in the middle of other structs, we use the `struct_group_tagged()` helper to create a new tagged `struct tty_buffer_hdr`. This structure groups together all the members of the flexible `struct tty_buffer` except the flexible array. As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct member currently causing trouble from `struct tty_buffer` to `struct tty_buffer_hdr`. We also want to ensure that when new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use `static_assert()`. This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes. This approach avoids having to implement `struct tty_buffer_hdr` as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future. Why not just have a separate structure and embed that in the places it is used? No duplication should be needed or am I missing something? I don't mind that, it would make this all much simpler and more obvious over time, and the tty layer needs all the "simplification" it can get :) +100. You can name the member hdr or even h. Another approach would be to get rid of sentinel completely. But that might be too hard. Have you looked into it? You should describe that above too. On the top of that: I remember I already looked into this when gcc14 was introduced and I was retracted by something else. Nevertheless, it took me quite a while to understand what the exact problem is and how you are doing the fix. Both from the patch (the main change in tty_bufhead is hidden behind whitespace changes) and especially from the description (you do not say the simple: tty_bufhead contains data[] in the middle due to embedded tty_buffer there). Both need to be improved. PS v2 was sent too early :P. thanks, -- js suse labs
Re: [PATCH][next] tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings
On 05. 02. 25, 7:49, Gustavo A. R. Silva wrote: If the above changes are better for you then I'll send a new patch. :) No, you are supposed to switch tty_buffer to tty_buffer_hdr too. thanks, -- js suse labs
Re: [PATCH v3][next] tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings
On 06. 02. 25, 4:39, Gustavo A. R. Silva wrote:
Currently, member `sentinel` in `struct tty_bufhead` is causing trouble
becase its type is `struct tty_buffer`, which is a flexible structure
--meaning it contains a flexible-array member. This combined with the
fact that `sentinel` is positioned in the middle of `struct tty_bufhead`,
THUMBS_UP
Suggested-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v3:
- Implement `struct tty_buffer_hdr` as a separate struct and embed it
into `struct tty_buffer`. Refactor the rest of the code, accordingly.
Better, but:
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 79f0ff94ce00..cd04a6567a33 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
...
@@ -576,11 +579,14 @@ int tty_insert_flip_string_and_push_buffer(struct
tty_port *port,
void tty_buffer_init(struct tty_port *port)
{
struct tty_bufhead *buf = &port->buf;
+ struct tty_buffer *buf_sentinel;
+
+ buf_sentinel = container_of(&buf->sentinel, struct tty_buffer, hdr);
Bah, so this is ugly and even dangerous if someone adds a member to
tty_buffer outside _hdr.
We should link headers in the list, it appears.
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -7,7 +7,7 @@
#include
#include
-struct tty_buffer {
+struct tty_buffer_hdr {
union {
struct tty_buffer *next;
struct llist_node free;
@@ -15,9 +15,13 @@ struct tty_buffer {
unsigned int used;
unsigned int size;
unsigned int commit;
- unsigned int lookahead; /* Lazy update on recv, can become less than
"read" */
+ unsigned int lookahead; /* Lazy update on recv, can become less than
"read" */
This is an unrelated/unwanted change.
thanks,
--
js
suse labs
