From: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
To: Richard Weinberger <richard@nod.at>
Cc: linux-can <linux-can@vger.kernel.org>
Subject: PATCH: Breaking UAPI change?
Date: Wed, 24 Mar 2021 20:27:46 +0100 [thread overview]
Message-ID: <20210324192746.GA7408@x1.vandijck-laurijssen.be> (raw)
In-Reply-To: <1135648123.112255.1616613706554.JavaMail.zimbra@nod.at>
> ----- Ursprüngliche Mail -----
> >> commit f5223e9eee65 ("can: extend sockaddr_can to include j1939 members")
> >> increased the size of
> >> struct sockaddr_can.
> >> This is a problem for applications which use recvfrom() with addrlen being
> >> sizeof(struct sockaddr_can)
> >> or sizeof(struct sockaddr).
> >> If such an application was built before the change it will no longer function
> >> correctly on newer kernels.
> >
> > This scenario was identified, and explicitely dealt with.
> > This requires a tiny bit different code, i.e. net/can/raw.c should use
> > REQUIRED_SIZE() instead of sizeof() for testing the size of the address
> > structure.
> >
> >> In fact I ran into such a scenario and found the said commit later that day.
> >
> > Looking to the v5.10 kernel (which I happen to have checked out),
> > your claim indeed seems true, the raw_recvmsg does not (raw_bind and
> > raw_sendmsg work correct, but that's not important for your problem).
> >
> >>
> >> Is this a known issue?
> >
> > It wasn't, until you found it :-)
>
> Thanks for the prompt reply!
>
> >> Or is this allowed and application must not use sizeof(struct sockaddr_can) as
> >> addrlen?
> >
> > sizeof(struct sockaddr_can). As you already mentioned, applications may have
> > been built
> > before the size increase, and so they should not be recompiled.
> >
> >
> >> If so, what is the proposed way to avoid future breakage?
> >
> > Your application should not change. Kernel must be fixed.
>
> Feel free to CC me when you submit a patch, I'll happily test it.
Here it goes.
Even not compile tested :-( on my v5.10, at this hour.
I spotted a similar problem in getsockname/getpeername calls.
The patch will test for minimum required size before touching anything,
later on, the maximum size will be evaluated.
Happy testing?
commit 124900109ca88d29382ef2e2b848d3a2f9d67b98
Author: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
Date: Wed Mar 24 20:08:50 2021
can raw: don't increase provided name length
The length of the buffer is known by the application,
not the kernel. Kernel is supposed to return only the
size of used bytes.
There is a minimum required size for the struct sockaddr_can
to be usefull for can_raw, so errors are returned when
the provided size is lower
Signed-off-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
diff --git a/net/can/raw.c b/net/can/raw.c
index 6ec8aa1d0da4..00d352ae221e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -475,7 +475,7 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
if (peer)
return -EOPNOTSUPP;
- memset(addr, 0, sizeof(*addr));
+ memset(addr, 0, CAN_REQUIRED_SIZE(*addr, ifindex));
addr->can_family = AF_CAN;
addr->can_ifindex = ro->ifindex;
@@ -806,6 +806,10 @@ static int raw_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
return sock_recv_errqueue(sk, msg, size,
SOL_CAN_RAW, SCM_CAN_RAW_ERRQUEUE);
+ if (msg->name && msg->msg_namelen <
+ CAN_REQUIRED_SIZE(struct sockaddr_can, ifindex))
+ return -EINVAL;
+
skb = skb_recv_datagram(sk, flags, noblock, &err);
if (!skb)
return err;
@@ -825,7 +829,8 @@ static int raw_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
if (msg->msg_name) {
__sockaddr_check_size(sizeof(struct sockaddr_can));
- msg->msg_namelen = sizeof(struct sockaddr_can);
+ if (msg->msg_namelen > sizeof(struct sockaddr_can))
+ msg->msg_namelen = sizeof(struct sockaddr_can);
memcpy(msg->msg_name, skb->cb, msg->msg_namelen);
}
next prev parent reply other threads:[~2021-03-24 19:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-24 15:30 Breaking UAPI change? Richard Weinberger
2021-03-24 19:01 ` Kurt Van Dijck
2021-03-24 19:21 ` Richard Weinberger
2021-03-24 19:27 ` Kurt Van Dijck [this message]
2021-03-24 19:47 ` PATCH: " Richard Weinberger
2021-03-24 20:26 ` Oliver Hartkopp
2021-03-24 21:59 ` Oliver Hartkopp
2021-03-25 8:01 ` Kurt Van Dijck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210324192746.GA7408@x1.vandijck-laurijssen.be \
--to=dev.kurt@vandijck-laurijssen.be \
--cc=linux-can@vger.kernel.org \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).