Re: [PATCH v5 1/2] media: rc: new driver for USB-UIRT device

[Johan Hovold <johan@kernel.org>]

On Fri, Jul 02, 2021 at 02:13:18PM +0100, Sean Young wrote:
> On Fri, Jul 02, 2021 at 12:42:18PM +0200, Johan Hovold wrote:
> > On Fri, Jun 18, 2021 at 11:18:46AM +0100, Sean Young wrote:
> > > This device uses an ftdi usb serial port, so this driver has a tiny
> > > amount of usb ftdi code. It would be preferable to connect this driver via
> > > serdev or line-discipline, but unfortunately neither support
> > > hotplugging yet.
> > > 
> > > See http://www.usbuirt.com/
> > > 
> > > Signed-off-by: Sean Young <sean@mess.org>

> > > +// read IR in raw mode
> > > +static void uirt_raw_mode(struct uirt *uirt, u32 offset, u32 len)
> > > +{
> > > +	uint i, duration;
> > > +
> > > +	for (i = offset; i < len; i++) {
> > > +		switch (uirt->rx_state) {
> > > +		case RX_STATE_INTERSPACE_HIGH:
> > > +			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
> > > +			break;
> > > +		case RX_STATE_INTERSPACE_LOW:
> > > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > > +			break;
> > > +		case RX_STATE_ON_HIGH:
> > > +			duration = uirt->in[i];
> > > +			if (duration == 0)
> > > +				duration = 1;
> > > +
> > > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +				.duration = duration * UNIT_US,
> > > +				.pulse = true,
> > > +			}));
> > > +
> > > +			uirt->rx_state = RX_STATE_OFF_HIGH;
> > > +			break;
> > > +		case RX_STATE_OFF_HIGH:
> > > +			if (uirt->in[i] == 0xff) {
> > > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +					.duration = IR_TIMEOUT,
> > > +					.timeout = true,
> > > +				}));
> > > +				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > > +				break;
> > > +			}
> > > +
> > > +			duration = uirt->in[i];
> > > +			if (duration == 0)
> > > +				duration = 1;
> > > +
> > > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +				.duration = duration * UNIT_US,
> > > +				.pulse = false,
> > > +			}));
> > > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > > +			break;
> > > +		default:
> > > +			WARN(1, "unreachable state");
> > 
> > This should probably be dev_warn_ratelimited() or similar. Judging from
> > a quick look a malicious device can end up triggering this.
> 
> Well, the other states can reached only by enabling the wideband receiver
> and then disabling it again, and then the driver state machine needs to
> be broken too. Just belt and braces.

Still looks like you can end up here since uirt->wideband isn't updated
until you get a reply from the device and the it's device input which
drives the uirt->rx_state transitions.

> > > +			uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	ir_raw_event_handle(uirt->rc);
> > > +}
> > > +
> > > +// Read IR in wideband mode. The byte stream is delivered in packets,
> > > +// and the values which come in two bytes may straddle two packets
> > > +static void uirt_wideband(struct uirt *uirt, u32 offset, u32 len)
> > > +{
> > > +	uint i, duration, carrier, pulses;
> > > +
> > > +	for (i = offset; i < len; i++) {
> > > +		switch (uirt->rx_state) {
> > > +		case RX_STATE_INTERSPACE_HIGH:
> > > +			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
> > > +			break;
> > > +		case RX_STATE_INTERSPACE_LOW:
> > > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > > +			break;
> > > +		case RX_STATE_ON_HIGH:
> > > +			uirt->high = uirt->in[i];
> > > +			uirt->rx_state = RX_STATE_ON_LOW;
> > > +			break;
> > > +		case RX_STATE_ON_LOW:
> > > +			// duration is in 400ns units
> > > +			duration = (uirt->high << 8) | uirt->in[i];
> > > +			uirt->last_duration = duration;
> > > +			// Convert to microseconds
> > > +			duration = DIV_ROUND_CLOSEST(duration * 2, 5);
> > > +			if (duration == 0)
> > > +				duration = 1;
> > > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +				.duration = duration,
> > > +				.pulse = true,
> > > +			}));
> > > +			uirt->rx_state = RX_STATE_FREQ_HIGH;
> > > +			break;
> > > +		case RX_STATE_FREQ_HIGH:
> > > +			if (uirt->in[i] & 0x80) {
> > > +				uirt->high = uirt->in[i] & 0x7f;
> > > +				uirt->rx_state = RX_STATE_FREQ_LOW;
> > > +				break;
> > > +			}
> > > +
> > > +			uirt->high = 0;
> > > +			fallthrough;
> > > +		case RX_STATE_FREQ_LOW:
> > > +			pulses = (uirt->high << 8) | uirt->in[i];
> > > +			if (pulses && uirt->last_duration) {
> > > +				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
> > > +					uirt->last_duration, pulses);
> > > +
> > > +				// calculate the Hz of pulses in duration 400ns units
> > > +				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
> > > +								uirt->last_duration * 4);
> > > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +					.carrier = carrier,
> > > +					.carrier_report = true,
> > > +				}));
> > > +			}
> > > +			uirt->rx_state = RX_STATE_OFF_HIGH;
> > > +			break;
> > > +		case RX_STATE_OFF_HIGH:
> > > +			if (uirt->in[i] == 0xff) {
> > > +				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +					.duration = IR_TIMEOUT,
> > > +					.timeout = true,
> > > +				}));
> > > +				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
> > > +			} else {
> > > +				uirt->high = uirt->in[i];
> > > +				uirt->rx_state = RX_STATE_OFF_LOW;
> > > +			}
> > > +			break;
> > > +		case RX_STATE_OFF_LOW:
> > > +			// Convert 400ns units to microseconds
> > > +			duration = DIV_ROUND_CLOSEST(((uirt->high << 8) | uirt->in[i]) * 2, 5);
> > > +			if (duration == 0)
> > > +				duration = 1;
> > > +			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
> > > +				.duration = duration,
> > > +				.pulse = false,
> > > +			}));
> > > +			uirt->rx_state = RX_STATE_ON_HIGH;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	ir_raw_event_handle(uirt->rc);
> > > +}
> > > +
> > > +static void uirt_response(struct uirt *uirt, u32 len)
> > > +{
> > > +	int offset = 2;
> > > +	int i;
> > > +
> > > +	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
> > > +
> > > +	// Do we have more IR to transmit and is Clear-To-Send set
> > > +	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
> > > +	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
> > 
> > Do you really need to handle this manually when you have hardware
> > assisted flow control enabled?
> 
> I had not considered this. I'll look into it.
> 
> > > +		u32 len;
> > > +		int err;
> > > +
> > > +		len = min_t(u32, uirt->tx_len, MAX_PACKET);
> > > +
> > > +		memcpy(uirt->out, uirt->tx_buf, len);
> > > +		uirt->urb_out->transfer_buffer_length = len;
> > > +
> > > +		uirt->tx_len -= len;
> > > +		uirt->tx_buf += len;
> > > +
> > > +		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
> > > +		if (err != 0)
> > > +			dev_warn(uirt->dev,
> > > +				 "failed to submit out urb: %d\n", err);
> > > +	}
> > > +
> > > +	// if we only have two bytes, it just gives us the serial line status
> > > +	if (len <= 2)
> > > +		return;
> > > +
> > > +	// We have to assume that the response to a command is at the beginning
> > > +	// of the packet. There is no way to distinguish IR data from command
> > > +	// responses other than the position in the byte stream.
> > > +	switch (uirt->cmd_state) {
> > > +	case CMD_STATE_GETVERSION:
> > > +		if (len >= 10) {
> > > +			// check checksum
> > > +			u8 checksum = 0;
> > > +
> > > +			for (i = 2; i < len; i++)
> > > +				checksum += uirt->in[i];
> > > +
> > > +			if (checksum != 0) {
> > > +				dev_err(uirt->dev, "checksum does not match: %*phN\n",
> > > +					len, uirt->in);
> > 
> > Should this not be ratelimited too in case you get out of sync?
> 
> The get version command is only issued during probe, so this can only occur
> once.

Sure, but you don't update the state in case this check fails so the
following packets of IR data could all end up here until the command
times out.

> > > +				return;
> > > +			}
> > > +
> > > +			dev_info(uirt->dev,
> > > +				 "USB-UIRT firmware v%u.%u protocol v%u.%u %04u-%02u-%02u",
> > 
> > Missing '\n' and in a lot of other printks throughout.
> 
> Yes, good point. I'll fix this.
> 
> > > +				 uirt->in[2], uirt->in[3], uirt->in[4], uirt->in[5],
> > > +				 uirt->in[8] + 2000, uirt->in[7], uirt->in[6]);
> > > +
> > > +			complete(&uirt->cmd_done);
> > > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > > +			offset += 10;
> > > +		}
> > > +		break;
> > > +	case CMD_STATE_DOTXRAW:
> > > +	case CMD_STATE_STREAMING_TX:
> > > +	case CMD_STATE_SETMODERAW:
> > > +	case CMD_STATE_SETMODEWIDEBAND:
> > > +		if (len >= 3) {
> > > +			switch (uirt->in[2]) {
> > > +			case 0x20:
> > > +				// 0x20 transmitting is expected during streaming tx
> > > +				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
> > > +					return;
> > > +
> > > +				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
> > > +					complete(&uirt->cmd_done);
> > > +				else
> > > +					dev_err(uirt->dev, "device transmitting");
> > 
> > I think most of these printks need to be ratelimited or dev_dbg() since
> > bad input input can trigger them.
> 
> All of these occur only as a response to an user space command, like transmit
> or switching wideband/narrowband receiver. These command are not issued very
> often, usually only in response to someone running ir-ctl(1) or so.

But a broken/malicious device, or if things just get out of sync, could
end up triggering these messages repeatedly until the commands time out
after five seconds, right?

> > Another missing newline, but please fix throughout.
> 
> Absolutely.
> 
> > > +				break;
> > > +			case 0x21:
> > > +				if (uirt->tx_len) {
> > > +					dev_err(uirt->dev, "tx completed with %u left to send",
> > > +						uirt->tx_len);
> > > +				} else {
> > > +					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
> > > +						uirt->wideband = false;
> > > +					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
> > > +						uirt->wideband = true;
> > > +
> > > +					complete(&uirt->cmd_done);
> > > +				}
> > > +				break;
> > > +			case 0x80:
> > > +				dev_err(uirt->dev, "checksum error");
> > > +				break;
> > > +			case 0x81:
> > > +				dev_err(uirt->dev, "timeout");
> > > +				break;
> > > +			case 0x82:
> > > +				dev_err(uirt->dev, "command error");
> > > +				break;
> > > +			default:
> > > +				dev_err(uirt->dev, "unknown response");
> > > +			}
> > > +
> > > +			uirt->cmd_state = CMD_STATE_IRDATA;
> > > +			offset += 1;
> > > +		}
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	if (uirt->wideband)
> > > +		uirt_wideband(uirt, offset, len);
> > > +	else
> > > +		uirt_raw_mode(uirt, offset, len);
> > > +}

Johan