netdev - Re: [PATCH 1/1] [DCCP][QPOLICY]: External interface changes

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] [day] [month] [year] [list]

Message-Id: <200804282329.48669.tomasz@grobelny.oswiecenia.net>
Date:	Mon, 28 Apr 2008 23:29:48 +0200
From:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
To:	Gerrit Renker <gerrit@....abdn.ac.uk>
Cc:	acme@...hat.com, dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] [DCCP][QPOLICY]: External interface changes

Dnia Monday 28 of April 2008, Gerrit Renker napisał:
> | @@ -721,9 +721,7 @@ static int dccp_msghdr_parse(struct msghdr *msg,
> | __u32 **priority)
> |
> |  		switch (cmsg->cmsg_type) {
> |  		case DCCP_SCM_PRIORITY:
> | -			if (cmsg->cmsg_len != CMSG_LEN(sizeof(__u32)))
> | -				return -EINVAL;
> | -			*priority = (__u32 *)CMSG_DATA(cmsg);
> | +			*cmsg_qpolicy = cmsg;
> |  			break;
> |  		default:
> |  			return -EINVAL;
>
> Here the cmsg is exported to any current qpolicy. The disadvantages
> are	that * there is no longer any length check, the functions using this
> interface have in the worst case no idea how much the user wanted to pass,
> so this is dangerous (compare also RFC 3542, 20.2);
The information about size can be retrieved in specific qpolicy. It is not 
CMSG_DATA(cmsg) that is passed to qpolicy but cmsg itself along with cmsg_len 
field.

>  * the separation-of-concerns becomes unclear: now part of the parsing is
> done in dccp_msg_hdr_parse() and another part in some of the
> qpolicy_xxx_push() routines, so one routine does part of the work of
> another;
That is a problem. So I think all the code should be moved to 
qpolicy_prio_push().

>  * each qpolicy_xxx_push() routine needs to have a cmsghdr* argument even
> if it is not used.
>
Before the change I could say: why call dccp_msghdr_parse() if choosen policy 
cannot make use of it's result.
No matter what we will always have either "unnecessary code", "unnecessary 
parameter" or something else that is "unnecessary" simply because simple 
policy is, well... simple. But it has to be so to keep interfaces clean.

> It is however possible to reach the same goal with just a small
> modification: * so far only one SCM message (DCCP_SCM_PRIORITY) has been
> defined; * maintainers generally accepted the use of skb->priority while on
> layer 4; * hence for all sub-variants of a "prio" policy (strict/partial
> orderings), one can use DCCP_SCM_PRIORITY, in combination with
> skb->priority; * for different policies, define a different DCCP_SCM_xxx
> message and store the cmsg data in a different field (to be decided);
>  * this has the advantage that type/length checking is all in one place,
>    so that the qpolicy routines need to do qpolicy only.
>
I would use different DCCP_SCM_xxx for different parameters. So that one 
qpolicy could use more than one DCCP_SCM_xxx. And parsing of specific 
DCCP_SCM_xxx should IMHO happen in qpolicy_*_push().

> | --- a/net/dccp/qpolicy.c
> | +++ b/net/dccp/qpolicy.c
> | @@ -57,8 +58,13 @@ static struct sk_buff *qpolicy_prio_worst_skb(struct
> | sock *sk) return worst;
> |  }
> |
> | -static void qpolicy_prio_push(struct sock *sk, struct sk_buff *skb)
> | +static void qpolicy_prio_push(struct sock *sk, struct sk_buff *skb,
> | +				struct cmsghdr *cmsg)
> |  {
> | +	struct dccp_qpolicy_prio_info info;
> | +	memcpy(&info, CMSG_DATA(cmsg), min(cmsg->cmsg_len, sizeof(info)));
>
> That is the problem I see - `info' could end up with garbled data, if the
> user had accidentally passed `u32' instead of CMSG_LEN(sizeof priority).
>
Right, but this could easily be fixed by initializing info prior to memcpy. 
Then only data provided by user would override defaults. But see below.

> The CMSG_LEN() ensures an additional check - that the user is in fact using
> proper encapsulation via CMSG_xxx macros and not via
> msg_control/msg_controllen directly. The latter leads to hard-to-debug
> problems: in my case, I found this works - by accident - on 32-bit Intel,
> but will fail on 64-bit computers in compatibility mode.
>
Then min(cmsg->cmsg_len, sizeof(info)) could be changed to min(CMSG_LEN(cmsg), 
sizeof(info)), I see no problem with that.

That said I think that multiple DCCP_SCM_xxx per policy might be a better 
approach.
-- 
Regards,
Tomasz Grobelny
--
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