@@ -546,10 +546,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,return-EFAULT;}+rtnl_lock();lock_sock(sk);-if(ro->bound&&ro->ifindex)+if(ro->bound&&ro->ifindex){dev=dev_get_by_index(sock_net(sk),ro->ifindex);+if(!dev){+if(count>1)+kfree(filter);+err=-ENODEV;+gotoout_fil;+}+}if(ro->bound){/* (try to) register the new filters */
@@ -588,6 +596,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,dev_put(dev);release_sock(sk);+rtnl_unlock();break;
@@ -600,10 +609,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,err_mask&=CAN_ERR_MASK;+rtnl_lock();lock_sock(sk);-if(ro->bound&&ro->ifindex)+if(ro->bound&&ro->ifindex){dev=dev_get_by_index(sock_net(sk),ro->ifindex);+if(!dev){+err=-ENODEV;+gotoout_err;+}+}/* remove current error mask */if(ro->bound){
@@ -627,6 +642,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,dev_put(dev);release_sock(sk);+rtnl_unlock();break;
@@ -546,10 +546,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,return-EFAULT;}+rtnl_lock();lock_sock(sk);-if(ro->bound&&ro->ifindex)+if(ro->bound&&ro->ifindex){dev=dev_get_by_index(sock_net(sk),ro->ifindex);+if(!dev){
+ if (count > 1)
+ kfree(filter);
This was NOT suggested!
I've been talking about removing the other kfree() "improvement" you
suggested.
The kfree() should only be done when ro->bound and ro->ifindex are cleared.
So when you remove these two lines it should be ok.
Please try to increase the context in the diff.
Thanks,
Oliver
quoted hunk
+ err = -ENODEV;
+ goto out_fil;
+ }
+ }
if (ro->bound) {
/* (try to) register the new filters */
@@ -588,6 +596,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, dev_put(dev); release_sock(sk);+ rtnl_unlock(); break;
@@ -600,10 +609,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, err_mask &= CAN_ERR_MASK;+ rtnl_lock(); lock_sock(sk);- if (ro->bound && ro->ifindex)+ if (ro->bound && ro->ifindex) { dev = dev_get_by_index(sock_net(sk), ro->ifindex);+ if (!dev) {+ err = -ENODEV;+ goto out_err;+ }+ } /* remove current error mask */ if (ro->bound) {
@@ -627,6 +642,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, dev_put(dev); release_sock(sk);+ rtnl_unlock(); break;
@@ -546,10 +546,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
return -EFAULT;
}
+ rtnl_lock();
lock_sock(sk);
- if (ro->bound && ro->ifindex)
+ if (ro->bound && ro->ifindex) {
dev = dev_get_by_index(sock_net(sk), ro->ifindex);
+ if (!dev) {
quoted
+ if (count > 1)
+ kfree(filter);
This was NOT suggested!
I've been talking about removing the other kfree() "improvement" you suggested.
The kfree() should only be done when ro->bound and ro->ifindex are cleared.
So when you remove these two lines it should be ok.
Please try to increase the context in the diff.
Thanks,
Oliver
Sorry, I am a little confused.
The following codes are the latest raw_setsockopt function realization(ignore some non-key parts)
with my patch. Now we assume the condition that count more than 1, ro->bound and ro->ifindex
are not zero, dev_get_by_index() will return NULL. We analyze the code logic.
static int raw_setsockopt(struct socket *sock, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
......
struct can_filter *filter = NULL;
......
switch (optname) {
case CAN_RAW_FILTER:
......
if (count > 1) {
/* filter does not fit into dfilter => alloc space */
filter = memdup_sockptr(optval, optlen); // filter point to a heap memory
if (IS_ERR(filter))
return PTR_ERR(filter);
} else if (count == 1) {
......
}
rtnl_lock();
lock_sock(sk);
if (ro->bound && ro->ifindex) {
dev = dev_get_by_index(sock_net(sk), ro->ifindex);
/*
* dev == NULL is exception. The function will exit abnormally.
* Memory pointed by filer does not forward to anyone for maintenance.
* If we do not kfree(filter) here, memory will be leaked after function exit.
*/
if (!dev) {
if (count > 1)
kfree(filter);
err = -ENODEV;
goto out_fil;
}
}
if (ro->bound) {
/* (try to) register the new filters */
if (count == 1)
err = raw_enable_filters(sock_net(sk), dev, sk,
&sfilter, 1);
else
err = raw_enable_filters(sock_net(sk), dev, sk,
filter, count);
if (err) {
if (count > 1)
kfree(filter);
goto out_fil;
}
/* remove old filter registrations */
raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
ro->count);
}
/* remove old filter space */
if (ro->count > 1)
kfree(ro->filter);
/* link new filters to the socket */
if (count == 1) {
/* copy filter data for single filter */
ro->dfilter = sfilter;
filter = &ro->dfilter;
}
ro->filter = filter;
ro->count = count;
out_fil:
if (dev)
dev_put(dev);
release_sock(sk);
rtnl_unlock();
break;
......
return err;
}
So I think my modification is right. Thank you.
@@ -546,10 +546,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,return-EFAULT;}+rtnl_lock();lock_sock(sk);-if(ro->bound&&ro->ifindex)+if(ro->bound&&ro->ifindex){dev=dev_get_by_index(sock_net(sk),ro->ifindex);+if(!dev){+if(count>1)+kfree(filter);+err=-ENODEV;+gotoout_fil;+}+}if(ro->bound){/* (try to) register the new filters */
@@ -588,6 +596,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,dev_put(dev);release_sock(sk);+rtnl_unlock();break;
@@ -600,10 +609,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,err_mask&=CAN_ERR_MASK;+rtnl_lock();lock_sock(sk);-if(ro->bound&&ro->ifindex)+if(ro->bound&&ro->ifindex){dev=dev_get_by_index(sock_net(sk),ro->ifindex);+if(!dev){+err=-ENODEV;+gotoout_err;+}+}/* remove current error mask */if(ro->bound){
@@ -627,6 +642,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,dev_put(dev);release_sock(sk);+rtnl_unlock();break;