[CRIU] [PATCH v3 2/5] net/sysctl: add sysctl_igmp_link_local_mcast_reports_safe check

[Pavel Emelyanov]

On 07/18/2016 11:45 AM, Pavel Tikhomirov wrote:
> 
> 
> On 07/15/2016 08:36 PM, Pavel Emelyanov wrote:
>> On 07/14/2016 04:51 PM, Pavel Tikhomirov wrote:
>>> In Linux v4.3 commit df2cf4a78e48 ("IGMP: Inhibit reports for local
>>> multicast groups") sysctl igmp_link_local_mcast_reports was introduced
>>> in ipv4_net_table.
>>>
>>> And in ipv4_net_table it's data was initialized to point on
>>> sysctl_igmp_llm_reports variable. That was so before commit
>>> 87a8a2ae65b7 ("igmp: Namespaceify igmp_llm_reports sysctl knob").
>>>
>>> So next it's data pointer is shifted to the offset of current
>>> netnamespace relative to init_net in ipv4_sysctl_init_net function.
>>> But that is completely wrong if variable is not net-namespaced, so we
>>> get random kernel address and can write/read to/from it one int, that
>>> can lead to memory corruption and crashes in random places in kernel.
>>>
>>> So conclusion is: we can not touch
>>> /proc/sys/net/ipv4/igmp_link_local_mcast_reports in v4.3-v4.5 between
>>> those two patches.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1352177
>>
>> This looks like a workaround for a known and fixed bug in the kernel
>> which is not criu-specific. Is it? If so, I don't see much point in
>> the patch, if someone drives into this problem, we just suggest one
>> to add the fixing patch to kernel.
> 
> Ok but that mean criu will make F23 crash as latest kernel there is 
> 4.5.7-202.fc23. Men from redhat seem do not want to fix it in F23.

I see.

I don't want to introduce the kernel version checker just for this
little problem. What if we make config option that doesn't C/R
this sysctl that will only be turned on by Fedora? Adrian, what
do you think?

-- Pavel

>>
>>> https://jira.sw.ru/browse/PSBM-48397
>>>
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>> ---
>>>  criu/include/kerndat.h |  1 +
>>>  criu/kerndat.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 41 insertions(+)
>>>
>>> diff --git a/criu/include/kerndat.h b/criu/include/kerndat.h
>>> index 0a5cd4b..9bd1625 100644
>>> --- a/criu/include/kerndat.h
>>> +++ b/criu/include/kerndat.h
>>> @@ -37,6 +37,7 @@ struct kerndat_s {
>>>  	bool has_compat_sigreturn;
>>>  	enum pagemap_func pmap;
>>>  	unsigned int has_xtlocks;
>>> +	bool sysctl_igmp_link_local_mcast_reports_safe;
>>>  };
>>>
>>>  extern struct kerndat_s kdat;
>>> diff --git a/criu/kerndat.c b/criu/kerndat.c
>>> index 04a355b..2a02678 100644
>>> --- a/criu/kerndat.c
>>> +++ b/criu/kerndat.c
>>> @@ -7,6 +7,7 @@
>>>  #include <sys/mman.h>
>>>  #include <errno.h>
>>>  #include <sys/syscall.h>
>>> +#include <sys/utsname.h>
>>>
>>>  #include "log.h"
>>>  #include "bug.h"
>>> @@ -460,6 +461,41 @@ static int kerndat_compat_restore(void)
>>>  	return 0;
>>>  }
>>>
>>> +static int kerndat_sysctl_igmp_link_local_mcast_reports_safe(void)
>>> +{
>>> +	int ret;
>>> +	struct utsname buf;
>>> +	char smajor[10], sminor[10];
>>> +	int major, minor;
>>> +
>>> +	kdat.sysctl_igmp_link_local_mcast_reports_safe = false;
>>> +
>>> +	ret = uname(&buf);
>>> +	if (ret) {
>>> +		pr_perror("Failed uname syscall");
>>> +		return 1;
>>> +	}
>>> +
>>> +	ret = sscanf(buf.release, "%[^'.'].%[^'.']", smajor, sminor);
>>> +	if (ret != 2) {
>>> +		pr_perror("Failed to parse major/minor from uname");
>>> +		return 1;
>>> +	}
>>> +
>>> +	major = atoi(smajor);
>>> +	minor = atoi(sminor);
>>> +
>>> +	if (!(major == 4 && minor >= 3 && minor <= 5))
>>> +		/*
>>> +		 * In Linux v4.3 commit df2cf4a78e48 ("IGMP: Inhibit reports for local multicast groups")
>>> +		 * buggy sysctl is added, it allows to write at random memory offsets from netns
>>> +		 * In Linux v4.6 commit 87a8a2ae65b7 ("igmp: Namespaceify igmp_llm_reports sysctl knob") it is fixed.
>>> +		 */
>>> +		kdat.sysctl_igmp_link_local_mcast_reports_safe = true;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  int kerndat_init(void)
>>>  {
>>>  	int ret;
>>> @@ -485,6 +521,8 @@ int kerndat_init(void)
>>>  		ret = kerndat_iptables_has_xtlocks();
>>>  	if (!ret)
>>>  		ret = kerndat_compat_restore();
>>> +	if (!ret)
>>> +		ret = kerndat_sysctl_igmp_link_local_mcast_reports_safe();
>>>
>>>  	kerndat_lsm();
>>>
>>> @@ -516,6 +554,8 @@ int kerndat_init_rst(void)
>>>  		ret = kerndat_iptables_has_xtlocks();
>>>  	if (!ret)
>>>  		ret = kerndat_compat_restore();
>>> +	if (!ret)
>>> +		ret = kerndat_sysctl_igmp_link_local_mcast_reports_safe();
>>>
>>>  	kerndat_lsm();
>>>
>>>
>>
>