linux-kernel - Re: [PATCH v2 2/2] tty: add rpmsg driver

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: <20190516182058.2a565748@alans-desktop>
Date:   Thu, 16 May 2019 18:20:58 +0100
From:   Alan Cox <gnomes@...rguk.ukuu.org.uk>
To:     Arnaud Pouliquen <arnaud.pouliquen@...com>
Cc:     Ohad Ben-Cohen <ohad@...ery.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        xiang xiao <xiaoxiang781216@...il.com>,
        <linux-kernel@...r.kernel.org>, <linux-remoteproc@...r.kernel.org>,
        Fabien DESSENNE <fabien.dessenne@...com>
Subject: Re: [PATCH v2 2/2] tty: add rpmsg driver


> +static int rpmsg_tty_data_handler(struct rpmsg_device *rpdev, void *data,
> +				  int len, void *priv, u32 src)
> +{
> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +	u8 *cbuf;
> +	int space;
> +
> +	dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> +
> +	if (!len)
> +		return 0;
> +
> +	dev_dbg(&rpdev->dev, "space available: %d\n",
> +		tty_buffer_space_avail(&cport->port));
> +
> +	space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> +	if (space <= 0) {
> +		dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> +		return -ENOMEM;
> +	}

Really bad idea.

1. It's not an 'error'. It's normal that in the case the consumer is
blocked you can run out of processing space

2. You will trigger this when being hit by a very large fast flow of data
so responding by burning CPU spewing messages (possibly even out of this
tty) is bad.

It's not a bug - just keep statistics and throw it away. If anyone cares
they will do flow control.


> +
> +static int rpmsg_tty_write_control(struct tty_struct *tty, u8 ctrl, u8 *values,
> +				   unsigned int n_value)
> +{
> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +	struct rpmsg_tty_payload *msg;
> +	struct rpmsg_tty_ctrl *m_ctrl;
> +	struct rpmsg_device *rpdev;
> +	unsigned int msg_size;
> +	int ret;
> +
> +	if (!cport) {
> +		dev_err(tty->dev, "cannot get cport\n");
> +		return -ENODEV;
> +	}
> +
> +	rpdev = cport->rpdev;
> +
> +	msg_size = sizeof(*msg) + sizeof(*m_ctrl) + n_value;
> +	msg = kzalloc(msg_size, GFP_KERNEL);


Out of memory check ?

> +static int rpmsg_tty_write(struct tty_struct *tty, const u8 *buf, int len)
> +{
> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +	struct rpmsg_device *rpdev;
> +	int msg_size, msg_max_size, ret = 0;
> +	int cmd_sz = sizeof(struct rpmsg_tty_payload);
> +	u8 *tmpbuf;
> +
> +	if (!cport) {
> +		dev_err(tty->dev, "cannot get cport\n");
> +		return -ENODEV;
> +	}
> +
> +	/* If cts not set, the message is not sent*/
> +	if (!cport->cts)
> +		return 0;
> +
> +	rpdev = cport->rpdev;
> +
> +	dev_dbg(&rpdev->dev, "%s: send msg from tty->index = %d, len = %d\n",
> +		__func__, tty->index, len);
> +	if (!buf) {
> +		dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> +		return -ENOMEM;
> +	}
> +
> +	msg_max_size = rpmsg_get_buf_payload_size(rpdev->ept);
> +	if (msg_max_size < 0)
> +		return msg_max_size;
> +
> +	msg_size = min(len + cmd_sz, msg_max_size);
> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);

Allocation failure check ?