Hello Lin Ma,
thanks for looking into this!
On 25.11.21 09:46, Lin Ma wrote:
The CAN_RAW_FILTER command allows the user to pass optlen as value 0. It
is expected to clear out the filer
static int raw_setsockopt(...) {
struct can_filter *filter = NULL;
...
// count = 0 if optlen = 0
count = optlen / sizeof(struct can_filter);
...
ro->filter = filter;
ro->count = count;
...
}
However, if this sock is bound to a device, bad thing happens as the
current check is failed to prevent the NULL Pointer Dereference.
static int raw_setsockopt(...) {
...
if (ro->bound) {
/* (try to) register the new filters */
if (count == 1)
err = raw_enable_filters(sock_net(sk), dev, sk,
&sfilter, 1);
else
// count = 0 can enter here while filter = NULL
// which is unexpected
err = raw_enable_filters(sock_net(sk), dev, sk,
filter, count);
...
}
This patch supplements the comparison with additional check.
Fixes: commit c18ce101f2e4 ("[CAN]: Add raw protocol")
Signed-off-by: Lin Ma <linma@xxxxxxxxxx>
---
net/can/raw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/can/raw.c b/net/can/raw.c
index 7105fa4824e4..590df2da081c 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -564,7 +564,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
if (count == 1)
err = raw_enable_filters(sock_net(sk), dev, sk,
&sfilter, 1);
- else
+ else if (count > 1)
err = raw_enable_filters(sock_net(sk), dev, sk,
filter, count);
if (err) {
AFAICS this additional check is not needed.
Please look into raw_enable_filters():
static int raw_enable_filters(struct net *net, struct net_device *dev,
struct sock *sk, struct can_filter *filter,
int count)
{
int err = 0;
int i;
for (i = 0; i < count; i++) {
^^^^^^^^^
This check ( 0 < 0 ) is never true in your described use-case. Therefore
we do not enter the for-loop and the pointer to the filter is never
dereferenced.
Or do I overlook something?
E.g. the can-utils tool 'cansend' is setting the filter to NULL:
https://github.com/linux-can/can-utils/blob/master/cansend.c#L157
But it does not crash - just double checked today ;-)
Best regards,
Oliver
err = can_rx_register(net, dev, filter[i].can_id,
filter[i].can_mask,
raw_rcv, sk, "raw", sk);
if (err) {
/* clean up successfully registered filters */
while (--i >= 0)
can_rx_unregister(net, dev,
filter[i].can_id,
filter[i].can_mask,
raw_rcv, sk);
break;
}
}
return err;
}
