* [PATCH] Truncate UTS_RELEASE for rxrpc version @ 2023-05-25 21:13 Kenny Ho 2023-05-25 22:09 ` David Howells 0 siblings, 1 reply; 5+ messages in thread From: Kenny Ho @ 2023-05-25 21:13 UTC (permalink / raw) To: David Laight, Jakub Kicinski, Andrew Lunn, Marc Dionne, Kenny Ho, David Howells, David S. Miller, Eric Dumazet, Paolo Abeni, linux-afs, netdev, linux-kernel, alexander.deucher Cc: Kenny Ho UTS_RELEASE has maximum length of 64 which can cause rxrpc_version to exceed the 65 byte message limit. Per https://web.mit.edu/kolya/afs/rx/rx-spec "If a server receives a packet with a type value of 13, and the client-initiated flag set, it should respond with a 65-byte payload containing a string that identifies the version of AFS software it is running." Current implementation causes compile error when WERROR is turned on and when UTS_RELEASE exceed the length of 49 (making the version string more than 64 characters.) Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> --- net/rxrpc/local_event.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c index 19e929c7c38b..90af6fbb9266 100644 --- a/net/rxrpc/local_event.c +++ b/net/rxrpc/local_event.c @@ -16,8 +16,6 @@ #include <generated/utsrelease.h> #include "ar-internal.h" -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; - /* * Reply to a version request */ @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, struct sockaddr_rxrpc srx; struct msghdr msg; struct kvec iov[2]; + static char rxrpc_version_string[65]; size_t len; int ret; @@ -38,6 +37,12 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, if (rxrpc_extract_addr_from_skb(&srx, skb) < 0) return; + if (!rxrpc_version_string[0]) + snprintf(rxrpc_version_string, + sizeof(rxrpc_version_string), + "linux-%.49s AF_RXRPC", + UTS_RELEASE); + msg.msg_name = &srx.transport; msg.msg_namelen = srx.transport_len; msg.msg_control = NULL; -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Truncate UTS_RELEASE for rxrpc version 2023-05-25 21:13 [PATCH] Truncate UTS_RELEASE for rxrpc version Kenny Ho @ 2023-05-25 22:09 ` David Howells 2023-05-25 22:28 ` Kenny Ho ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: David Howells @ 2023-05-25 22:09 UTC (permalink / raw) To: Kenny Ho Cc: dhowells, David Laight, Jakub Kicinski, Andrew Lunn, Marc Dionne, Kenny Ho, David S. Miller, Eric Dumazet, Paolo Abeni, linux-afs, netdev, linux-kernel, alexander.deucher Kenny Ho <Kenny.Ho@amd.com> wrote: > @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, > struct sockaddr_rxrpc srx; > struct msghdr msg; > struct kvec iov[2]; > + static char rxrpc_version_string[65]; > size_t len; > int ret; > That's not thread-safe. If you have multiple endpoints each one of them could be overwriting the string at the same time. We can't guarantee that one wouldn't corrupt the other. There's also no need to reprint it every time; just once during module init will do. How about the attached patch instead? David --- rxrpc: Truncate UTS_RELEASE for rxrpc version UTS_RELEASE has a maximum length of 64 which can cause rxrpc_version to exceed the 65 byte message limit. Per the rx spec[1]: "If a server receives a packet with a type value of 13, and the client-initiated flag set, it should respond with a 65-byte payload containing a string that identifies the version of AFS software it is running." The current implementation causes a compile error when WERROR is turned on and/or UTS_RELEASE exceeds the length of 49 (making the version string more than 64 characters). Fix this by generating the string during module initialisation and limiting the UTS_RELEASE segment of the string does not exceed 49 chars. We need to make sure that the 64 bytes includes "linux-" at the front and " AF_RXRPC" at the back as this may be used in pattern matching. Fixes: 44ba06987c0b ("RxRPC: Handle VERSION Rx protocol packets") Reported-by: Kenny Ho <Kenny.Ho@amd.com> Link: https://lore.kernel.org/r/20230523223944.691076-1-Kenny.Ho@amd.com/ Signed-off-by: David Howells <dhowells@redhat.com> Link: https://web.mit.edu/kolya/afs/rx/rx-spec [1] cc: Marc Dionne <marc.dionne@auristor.com> cc: Andrew Lunn <andrew@lunn.ch> cc: David Laight <David.Laight@ACULAB.COM> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: linux-afs@lists.infradead.org cc: netdev@vger.kernel.org --- net/rxrpc/af_rxrpc.c | 1 + net/rxrpc/ar-internal.h | 1 + net/rxrpc/local_event.c | 11 ++++++++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c index 31f738d65f1c..da0b3b5157d5 100644 --- a/net/rxrpc/af_rxrpc.c +++ b/net/rxrpc/af_rxrpc.c @@ -980,6 +980,7 @@ static int __init af_rxrpc_init(void) BUILD_BUG_ON(sizeof(struct rxrpc_skb_priv) > sizeof_field(struct sk_buff, cb)); ret = -ENOMEM; + rxrpc_gen_version_string(); rxrpc_call_jar = kmem_cache_create( "rxrpc_call_jar", sizeof(struct rxrpc_call), 0, SLAB_HWCACHE_ALIGN, NULL); diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index 5d44dc08f66d..e8e14c6f904d 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -1068,6 +1068,7 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t, /* * local_event.c */ +void rxrpc_gen_version_string(void); void rxrpc_send_version_request(struct rxrpc_local *local, struct rxrpc_host_header *hdr, struct sk_buff *skb); diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c index 5e69ea6b233d..993c69f97488 100644 --- a/net/rxrpc/local_event.c +++ b/net/rxrpc/local_event.c @@ -16,7 +16,16 @@ #include <generated/utsrelease.h> #include "ar-internal.h" -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; +static char rxrpc_version_string[65]; // "linux-" UTS_RELEASE " AF_RXRPC"; + +/* + * Generate the VERSION packet string. + */ +void rxrpc_gen_version_string(void) +{ + snprintf(rxrpc_version_string, sizeof(rxrpc_version_string), + "linux-%.49s AF_RXRPC", UTS_RELEASE); +} /* * Reply to a version request ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Truncate UTS_RELEASE for rxrpc version 2023-05-25 22:09 ` David Howells @ 2023-05-25 22:28 ` Kenny Ho 2023-05-25 22:52 ` David Howells 2023-05-26 9:38 ` Simon Horman 2 siblings, 0 replies; 5+ messages in thread From: Kenny Ho @ 2023-05-25 22:28 UTC (permalink / raw) To: David Howells Cc: Kenny Ho, David Laight, Jakub Kicinski, Andrew Lunn, Marc Dionne, David S. Miller, Eric Dumazet, Paolo Abeni, linux-afs, netdev, linux-kernel, alexander.deucher On Thu, May 25, 2023 at 6:09 PM David Howells <dhowells@redhat.com> wrote: > There's also no need to reprint it every time; just once during module init > will do. How about the attached patch instead? This makes sense and looks fine to me. I don't know the proper etiquette here, but Acked-by: Kenny Ho <Kenny.Ho@amd.com> Kenny ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Truncate UTS_RELEASE for rxrpc version 2023-05-25 22:09 ` David Howells 2023-05-25 22:28 ` Kenny Ho @ 2023-05-25 22:52 ` David Howells 2023-05-26 9:38 ` Simon Horman 2 siblings, 0 replies; 5+ messages in thread From: David Howells @ 2023-05-25 22:52 UTC (permalink / raw) To: Kenny Ho Cc: dhowells, Kenny Ho, David Laight, Jakub Kicinski, Andrew Lunn, Marc Dionne, David S. Miller, Eric Dumazet, Paolo Abeni, linux-afs, netdev, linux-kernel, alexander.deucher Kenny Ho <y2kenny@gmail.com> wrote: > This makes sense and looks fine to me. I don't know the proper > etiquette here, but > Acked-by: Kenny Ho <Kenny.Ho@amd.com> If I'm not going to pick the patch up, I tend to use Acked-by when reviewing a patch that touches code I'm a listed maintainer for and Reviewed-by when it's code that I'm not a maintainer for... but the descriptions in: Documentation/process/submitting-patches.rst seem to leave a lot of overlap. David ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Truncate UTS_RELEASE for rxrpc version 2023-05-25 22:09 ` David Howells 2023-05-25 22:28 ` Kenny Ho 2023-05-25 22:52 ` David Howells @ 2023-05-26 9:38 ` Simon Horman 2 siblings, 0 replies; 5+ messages in thread From: Simon Horman @ 2023-05-26 9:38 UTC (permalink / raw) To: David Howells Cc: Kenny Ho, David Laight, Jakub Kicinski, Andrew Lunn, Marc Dionne, Kenny Ho, David S. Miller, Eric Dumazet, Paolo Abeni, linux-afs, netdev, linux-kernel, alexander.deucher On Thu, May 25, 2023 at 11:09:14PM +0100, David Howells wrote: > Kenny Ho <Kenny.Ho@amd.com> wrote: > > > @@ -30,6 +28,7 @@ static void rxrpc_send_version_request(struct rxrpc_local *local, > > struct sockaddr_rxrpc srx; > > struct msghdr msg; > > struct kvec iov[2]; > > + static char rxrpc_version_string[65]; > > size_t len; > > int ret; > > > > That's not thread-safe. If you have multiple endpoints each one of them could > be overwriting the string at the same time. We can't guarantee that one > wouldn't corrupt the other. > > There's also no need to reprint it every time; just once during module init > will do. How about the attached patch instead? > > David Thanks David ad Kenny, can we arrange for a formal posting of the patch below? I suspect it will languish otherwise. > --- > rxrpc: Truncate UTS_RELEASE for rxrpc version > > UTS_RELEASE has a maximum length of 64 which can cause rxrpc_version to > exceed the 65 byte message limit. > > Per the rx spec[1]: "If a server receives a packet with a type value of 13, > and the client-initiated flag set, it should respond with a 65-byte payload > containing a string that identifies the version of AFS software it is > running." > > The current implementation causes a compile error when WERROR is turned on > and/or UTS_RELEASE exceeds the length of 49 (making the version string more > than 64 characters). > > Fix this by generating the string during module initialisation and limiting > the UTS_RELEASE segment of the string does not exceed 49 chars. We need to > make sure that the 64 bytes includes "linux-" at the front and " AF_RXRPC" at > the back as this may be used in pattern matching. > > Fixes: 44ba06987c0b ("RxRPC: Handle VERSION Rx protocol packets") > Reported-by: Kenny Ho <Kenny.Ho@amd.com> > Link: https://lore.kernel.org/r/20230523223944.691076-1-Kenny.Ho@amd.com/ > Signed-off-by: David Howells <dhowells@redhat.com> > Link: https://web.mit.edu/kolya/afs/rx/rx-spec [1] > cc: Marc Dionne <marc.dionne@auristor.com> > cc: Andrew Lunn <andrew@lunn.ch> > cc: David Laight <David.Laight@ACULAB.COM> > cc: "David S. Miller" <davem@davemloft.net> > cc: Eric Dumazet <edumazet@google.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Paolo Abeni <pabeni@redhat.com> > cc: linux-afs@lists.infradead.org > cc: netdev@vger.kernel.org > --- > net/rxrpc/af_rxrpc.c | 1 + > net/rxrpc/ar-internal.h | 1 + > net/rxrpc/local_event.c | 11 ++++++++++- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c > index 31f738d65f1c..da0b3b5157d5 100644 > --- a/net/rxrpc/af_rxrpc.c > +++ b/net/rxrpc/af_rxrpc.c > @@ -980,6 +980,7 @@ static int __init af_rxrpc_init(void) > BUILD_BUG_ON(sizeof(struct rxrpc_skb_priv) > sizeof_field(struct sk_buff, cb)); > > ret = -ENOMEM; > + rxrpc_gen_version_string(); > rxrpc_call_jar = kmem_cache_create( > "rxrpc_call_jar", sizeof(struct rxrpc_call), 0, > SLAB_HWCACHE_ALIGN, NULL); > diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h > index 5d44dc08f66d..e8e14c6f904d 100644 > --- a/net/rxrpc/ar-internal.h > +++ b/net/rxrpc/ar-internal.h > @@ -1068,6 +1068,7 @@ int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t, > /* > * local_event.c > */ > +void rxrpc_gen_version_string(void); > void rxrpc_send_version_request(struct rxrpc_local *local, > struct rxrpc_host_header *hdr, > struct sk_buff *skb); > diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c > index 5e69ea6b233d..993c69f97488 100644 > --- a/net/rxrpc/local_event.c > +++ b/net/rxrpc/local_event.c > @@ -16,7 +16,16 @@ > #include <generated/utsrelease.h> > #include "ar-internal.h" > > -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC"; > +static char rxrpc_version_string[65]; // "linux-" UTS_RELEASE " AF_RXRPC"; > + > +/* > + * Generate the VERSION packet string. > + */ > +void rxrpc_gen_version_string(void) > +{ > + snprintf(rxrpc_version_string, sizeof(rxrpc_version_string), > + "linux-%.49s AF_RXRPC", UTS_RELEASE); > +} > > /* > * Reply to a version request > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-26 9:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-25 21:13 [PATCH] Truncate UTS_RELEASE for rxrpc version Kenny Ho 2023-05-25 22:09 ` David Howells 2023-05-25 22:28 ` Kenny Ho 2023-05-25 22:52 ` David Howells 2023-05-26 9:38 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).