All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-samsung-soc@vger.kernel.org, linux-input@vger.kernel.org,
	lars@opdenkamp.eu, linux@arm.linux.org.uk,
	Hans Verkuil <hansverk@cisco.com>, Kamil Debski <kamil@wypas.org>,
	Hans Verkuil <hans.verkuil@cisco.com>
Subject: Re: [PATCHv16 08/13] DocBook/media: add CEC documentation
Date: Fri, 17 Jun 2016 13:09:10 +0200	[thread overview]
Message-ID: <5763DA56.20402@xs4all.nl> (raw)
In-Reply-To: <20160617065028.7410ae46@recife.lan>

On 06/17/2016 11:50 AM, Mauro Carvalho Chehab wrote:
>>>> +	  <row>
>>>> +	    <entry><constant>CEC_MODE_MONITOR</constant></entry>
>>>> +	    <entry>0xe0</entry>
>>>> +	    <entry>Put the file descriptor into monitor mode. Can only be used in combination
>>>> +	    with <constant>CEC_MODE_NO_INITIATOR</constant>, otherwise &EINVAL; will be
>>>> +	    returned. In monitor mode all messages this CEC device transmits and all messages
>>>> +	    it receives (both broadcast messages and directed messages for one its logical
>>>> +	    addresses) will be reported. This is very useful for debugging.</entry>
>>>> +	  </row>
>>>> +	  <row>
>>>> +	    <entry><constant>CEC_MODE_MONITOR_ALL</constant></entry>
>>>> +	    <entry>0xf0</entry>
>>>> +	    <entry>Put the file descriptor into 'monitor all' mode. Can only be used in combination
>>>> +            with <constant>CEC_MODE_NO_INITIATOR</constant>, otherwise &EINVAL; will be
>>>> +            returned. In 'monitor all' mode all messages this CEC device transmits and all messages
>>>> +            it receives, including directed messages for other CEC devices will be reported. This
>>>> +	    is very useful for debugging, but not all devices support this. This mode requires that
>>>> +	    the <constant>CEC_CAP_MONITOR_ALL</constant> capability is set, and depending on the
>>>> +	    hardware, you may have to be root to select this mode.</entry>  
>>>
>>> Please mention the error codes when it fails.  
>>
>> Ack.
>>
>>> "and depending on the hardware, you may have to be root to select this mode."
>>>
>>> No. We should define if CAP_SYS_ADMIN (or maybe CAP_NET_ADMIN, with
>>> is required to set promiscuous mode for network) will be required or
>>> not and enforce it for all hardware.  
>>
>> Ack for CAP_SYS_ADMIN (or possible NET_ADMIN, I'll look into that).
> 
> Thanks.
> 
>>> IMHO, putting the device into monitor mode should require it.  
>>
>> No. The CEC 2.0 spec explicitly recommends that the CEC adapter should be able to
>> monitor all messages.
> 
> What the hardware should or should not do is one thing. The other
> thing is what permissions are needed in order to use such
> feature.
> 
> I don't doubt that a similar requirement exists on 802.x, or on some
> industry standard document requiring that all Ethernet hardware should 
> support promiscuous mode. Yet, for security reasons, enabling such feature 
> requires special caps on Linux.
> 
>> The problem is that 1) not all hardware supports this, and 2)
>> hardware that does support this tends to mention that it is for testing only and
>> it shouldn't be used otherwise.
>>
>> If the hardware is fine with allowing monitoring of all messages, then anyone
>> should be able to do that. 
> 
> Why?

The spec recommends that this is supported in order for applications to take advantage
of seeing such traffic to optimize their performance (i.e. by sniffing traffic you can see which
devices are there so you don't have to discover them). The underlying reason is that
the CEC bus is so hopelessly slow, so any mechanism that helps avoids having to use
the bus is good.

>From the HDMI 2.0 spec (section 11.4):

"Polling can and should be skipped if other CEC traffic shows that a device is present.
 Hence, a device should not poll a certain Logical Address within at least one Minimum
 Polling Period after the following CEC events occur between the device that is polling
 and the device whose Logical Address is to be polled:

 - A directly addressed message, sent to that Logical Address, was acknowledged.
 - A directly addressed message has been sent from that Logical Address.
 - A broadcast message has been sent from that Logical Address.

 It is recommended that, if the device is capable of monitoring CEC traffic directed to
 other devices, then this capability should also be used to further reduce the need for polling.
 In this case, such a device should not poll a certain Logical Address for at least one
 Minimum Polling Period after it detects that that Logical Address acknowledged a directed
 message initiated from any Logical Address, or any message was sent from that Logical Address."

I wouldn't have required CAP_*_ADMIN at all if it wasn't for the scary language in some of the
hardware specs that I have seen.

(see below before replying to this :-) )

> 
>> But if it comes with all these 'for testing only' caveats,
>> then I think it should should be protected against 'casual' use. Unfortunately they
>> never tell you why it should be used for testing only (overly cautious or could
>> something actually fail when in this mode?).
>>
>> The reality is that being able to monitor the CEC bus is extremely useful when debugging.
> 
> Well, it can be debugged as root. That's not an issue. The issue
> here is if sniffing the traffic by a normal user could leak
> something that should not usually be seen by a normal user, like
> a Netflix password, that was typed via the RC, for example.

Interesting example. I hadn't thought of that.

OK, so I'm going to change the code so CAP_SYS/NET_ADMIN is required for the
monitoring mode. It can always be relaxed later, but this password sniffing
example is quite convincing actually.

The HDMI 2.0 polling recommendation can still be met since this would be handled
in the CEC framework (not implemented today, but it is planned since it is fairly
easy to do so). Since that's all inside the kernel nothing is leaked to non-root
applications.

One area where I am uncertain is when remote control messages are received and
passed on by the framework to the RC input device.

Suppose the application is the one receiving a password, then that password appears
both in the input device and the cec device. What I think will be useful is if the
application can prevent the use of an input device to pass on remote control messages.

CEC_ADAP_S_LOG_ADDRS has a flags field that I intended for just that purpose.

Note that RC messages are always passed on to CEC followers even if there is an
input device since some RC messages have additional arguments that the rc subsystem
can't handle. Also I think that it is often easier to handle all messages from the
same CEC device instead of having to read from two devices (cec and input). I
actually considered removing the input support, but it turned out to be useful in
existing video streaming apps since they don't need to add special cec support to
handle remote control presses.

Question: is there a way for applications to get exclusive access to an input device?
Or can anyone always read from it?

So I am thinking that I add a flag to inhibit the use of the input device if set.

This would be in a follow-on patch and it's added to the TODO file in the initial
patch series.

Comments?

>> The simple MONITOR mode is different in that it requires no special hardware configuration.
>> But it won't be able to see directed messages between CEC devices other than us.
>>
>> On a related note: if an application tries to become initiator or follower and someone
>> else has already exclusive access, then EBUSY is returned. I based this on what happens
>> in V4L2 with the S_PRIORITY ioctl.
>>
>> However, I think EACCES is a much better error code. It's probably what S_PRIO should
>> have used as well.
>>
>> Do you agree?
> 
> Not really. EACCES sounds more like a permanent problem, while EBUSY
> is always a temporary issue.
> 
> The problem with EACESS is that the user may think that the file
> permissions at the /dev/cec? are wrong.

Hmm, OK. I'm not entirely convinced, but I don't think it is a big deal either.

Besides, now I don't need to change anything :-)

Regards,

	Hans

  reply	other threads:[~2016-06-17 11:09 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-29 13:52 [PATCHv16 00/13] HDMI CEC framework Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 01/13] input.h: add BUS_CEC type Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 02/13] HID: add HDMI CEC specific keycodes Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 03/13] rc: Add HDMI CEC protocol handling Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 04/13] cec: add HDMI CEC framework Hans Verkuil
2016-06-16 16:00   ` Mauro Carvalho Chehab
2016-06-25 12:27     ` Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 05/13] cec/TODO: add TODO file so we know why this is still in staging Hans Verkuil
2016-04-29 13:52   ` Hans Verkuil
2016-06-16 19:53   ` Mauro Carvalho Chehab
2016-04-29 13:52 ` [PATCHv16 06/13] cec: add compat32 ioctl support Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 07/13] cec.txt: add CEC framework documentation Hans Verkuil
2016-04-29 13:52   ` Hans Verkuil
2016-06-16 20:12   ` Mauro Carvalho Chehab
2016-06-17  7:22     ` Hans Verkuil
2016-06-17  8:55       ` Mauro Carvalho Chehab
2016-04-29 13:52 ` [PATCHv16 08/13] DocBook/media: add CEC documentation Hans Verkuil
2016-06-16 21:09   ` Mauro Carvalho Chehab
2016-06-17  7:58     ` Hans Verkuil
2016-06-17  9:50       ` Mauro Carvalho Chehab
2016-06-17 11:09         ` Hans Verkuil [this message]
2016-06-17 11:37           ` Mauro Carvalho Chehab
2016-06-18 16:25             ` Dmitry Torokhov
2016-04-29 13:52 ` [PATCHv16 09/13] cec: adv7604: add cec support Hans Verkuil
2016-06-16 21:17   ` Mauro Carvalho Chehab
2016-06-17  8:03     ` Hans Verkuil
2016-06-17  9:53       ` Mauro Carvalho Chehab
2016-04-29 13:52 ` [PATCHv16 10/13] cec: adv7842: " Hans Verkuil
2016-06-16 21:22   ` Mauro Carvalho Chehab
2016-06-17  8:06     ` Hans Verkuil
2016-06-17  8:06       ` Hans Verkuil
2016-06-17 10:12       ` Mauro Carvalho Chehab
2016-04-29 13:52 ` [PATCHv16 11/13] cec: adv7511: " Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 12/13] cec: s5p-cec: Add s5p-cec driver Hans Verkuil
2016-04-29 13:52 ` [PATCHv16 13/13] vivid: add CEC emulation Hans Verkuil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5763DA56.20402@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hans.verkuil@cisco.com \
    --cc=hansverk@cisco.com \
    --cc=kamil@wypas.org \
    --cc=lars@opdenkamp.eu \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=mchehab@s-opensource.com \
    --subject='Re: [PATCHv16 08/13] DocBook/media: add CEC documentation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.