netdev - Re: Regression: Recent networking (qdisc?) patches break irda_get_next

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: <48FFC438.1080505@ceibo.fiec.espol.edu.ec>
Date:	Wed, 22 Oct 2008 19:24:24 -0500
From:	Alex Villacís Lasso 
	<avillaci@...bo.fiec.espol.edu.ec>
To:	irda-users@...ts.sourceforge.net, netdev@...r.kernel.org,
	David Miller <davem@...emloft.net>
Subject: Re: Regression: Recent networking (qdisc?) patches break irda_get_next_speed()

David Miller escribió:
> From: Alex Villacís Lasso <avillaci@...bo.fiec.espol.edu.ec>
> Date: Tue, 21 Oct 2008 18:37:56 -0500
>
>   
>> So then, the bug is that the cb field in the struct sk_buff is being
>> interpreted as both a struct qdisc_skb_cb and an struct irda_skb_cb,
>> for the same instance of struct sk_buff. I have just started to
>> review the suggested patch, but it seems that 'struct qdisc_skb_cb'
>> was meant to be aliased against the data for other layers (as
>> suggested by the presence of a 'char data[]' field). If so, how come
>> only IrDA is affected? How come UDP, TCP, etc. not affected by this?
>> On the other hand, if qdisc_skb_cb was not meant to be aliased, then
>> the IrDA case was left out while converting the rest of the layers
>> so that they will skip over the member 'pkt_len' of the 'struct
>> qdisc_skb_cb'.
>>     
>
> The SKB control block is not aliased.
>
> Once the packet is given to dev_queue_xmit() the packet scheduler
> "owns" the control block of the SKB.
>
> What IRDA is doing is illegal, and breaks in other ways without the
> commit in question.
>
> IRDA cannot depend upon the SKB control block not changing across
> the dev_queue_xmit() call.
>
>   
Let me see if I understood. So the particular illegal thing the IRDA 
stack is doing is the access of the control block in the middle of the 
driver transmit routine (via irda_get_next_speed() and friends). This 
information should be stored somewhere else. Exactly *where* to store it 
is the main problem to solve.

What is the proper way (if any) to store per-packet parameters (other 
than the payload itself) which are specific to a particular layer (IrDA 
in this case) and which are needed by drivers in order to work 
correctly? The control block gets overwritten by the time the driver 
proc (hard_start_xmit) is called, so this approach is now ruled out. I 
was thinking about storing a copy of the parameters (struct irda_skb_cb) 
as a header within the payload itself (skb->data[]), but I am not sure 
about whether this approach is a good design decision. I am open to 
suggestions on where to place the parameters.

-- 
perl -e '$x=2.4;print sprintf("%.0f + %.0f = %.0f\n",$x,$x,$x+$x);'

--
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