netdev - Re: [PATCH 2/5] dccp: Auto-load (when supported) CCID plugins for negotiation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081216054416.GB6199@gerrit.erg.abdn.ac.uk>
Date:	Tue, 16 Dec 2008 06:44:16 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>, davem@...emloft.net,
	dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/5] dccp: Auto-load (when supported) CCID plugins for
	negotiation

| > +/**
| > + * ccid_request_module  -  Pre-load CCID module for later use
| > + * This should be called only from process context (e.g. during connection
| > + * setup) and is necessary for later calls to ccid_new (typically in software
| > + * interrupt), so that it has the modules available when they are needed.
| > + */
| > +static int ccid_request_module(u8 id)
| > +{
| > +	if (!in_atomic()) {
| 
| Shouldn't the above be in a BUG_ON? It looks strange to simply refuse to
| do that and return OK. 
| 
| > +		ccids_read_lock();
| > +		if (ccids[id] == NULL) {
| > +			ccids_read_unlock();
| > +			return request_module("net-dccp-ccid-%d", id);
| > +		}
| > +		ccids_read_unlock();
| > +	}
| > +	return 0;
| > +}
We can do this if you want, but in a way this is a suggestion for the
existing code, compare with the original from ccid_new() below

	ccids_read_lock();
#ifdef CONFIG_MODULES
	if (ccids[id] == NULL) {
		/* We only try to load if in process context */
		ccids_read_unlock();
		if (gfp & GFP_ATOMIC)
			goto out;
		request_module("net-dccp-ccid-%d", id);
		ccids_read_lock();
	}
#endif

==> gfp is atomic when called from dccp_feat_update_ccid(), which is why
    in the previous feature-negotiation code DCCP crashed when trying to
    negotiate any CCID module that had not been loaded:
    
	new_ccid = ccid_new(new_ccid_nr, sk, rx, GFP_ATOMIC);

    meant that the module would not be reloaded.

==> On the other hand, when called via ccid_hc_{rx,tx}_new(), gfp=GFP_KERNEL
    and so an attempted module load would succeed. This explains why loading
    worked when starting with the default (non-negotiated) CCIDs.



==> So how do we remain? I am ok turning the above into a BUG_ON.
    As an alternative, since the function is only called in a single
    place (the initialisation of feat.c), we could consider removing the
    function entirely and merge its contents into dccp_feat_init().
    Would that be better?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html