[PATCH 1/3] tty: n_gsm: fix buffer over-read in gsm_dlci_data()

[PATCH 1/3] tty: n_gsm: fix buffer over-read in gsm_dlci_data()

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

'len' is decreased after each octet that has its EA bit set to 0, which
means that the value is encoded with additional octets. However, the final
octet does not decreases 'len' which results in 'len' being one byte too
long. A buffer over-read may occur in tty_insert_flip_string() as it tries
to read one byte more than the passed content size of 'data'.
Decrease 'len' also for the final octet which has the EA bit set to 1 to
write the correct number of bytes from the internal receive buffer to the
virtual tty.

Fixes: 2e124b4a390c ("TTY: switch tty_flip_buffer_push")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index a38b922bcbc1..9b0b435cf26e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1658,6 +1658,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
 			if (len == 0)
 				return;
 		}
+		len--;
 		slen++;
 		tty = tty_port_tty_get(port);
 		if (tty) {
-- 
2.34.1

[PATCH 2/3] tty: n_gsm: fix mux activation issues in gsm_config()

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The current implementation activates the mux if it was restarted and opens
the control channel if the mux was previously closed and we are now acting
as initiator instead of responder, which is the default setting.
This has two issues.
1) No mux is activated if we keep all default values and only switch to
initiator. The control channel is not allocated but will be opened next
which results in a NULL pointer dereference.
2) Switching the configuration after it was once configured while keeping
the initiator value the same will not reopen the control channel if it was
closed due to parameter incompatibilities. The mux remains dead.

Fix 1) by always activating the mux if it is dead after configuration.
Fix 2) by always opening the control channel after mux activation.

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9b0b435cf26e..bcb714031d69 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2352,6 +2352,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
 
 static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 {
+	int ret = 0;
 	int need_close = 0;
 	int need_restart = 0;
 
@@ -2419,10 +2420,13 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	 * FIXME: We need to separate activation/deactivation from adding
 	 * and removing from the mux array
 	 */
-	if (need_restart)
-		gsm_activate_mux(gsm);
-	if (gsm->initiator && need_close)
-		gsm_dlci_begin_open(gsm->dlci[0]);
+	if (gsm->dead) {
+		ret = gsm_activate_mux(gsm);
+		if (ret)
+			return ret;
+		if (gsm->initiator)
+			gsm_dlci_begin_open(gsm->dlci[0]);
+	}
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 2/3] tty: n_gsm: fix mux activation issues in gsm_config()

[Jiri Slaby]

On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The current implementation activates the mux if it was restarted and opens
> the control channel if the mux was previously closed and we are now acting
> as initiator instead of responder, which is the default setting.
> This has two issues.
> 1) No mux is activated if we keep all default values and only switch to
> initiator. The control channel is not allocated but will be opened next
> which results in a NULL pointer dereference.
> 2) Switching the configuration after it was once configured while keeping
> the initiator value the same will not reopen the control channel if it was
> closed due to parameter incompatibilities. The mux remains dead.
> 
> Fix 1) by always activating the mux if it is dead after configuration.
> Fix 2) by always opening the control channel after mux activation.
> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>   drivers/tty/n_gsm.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 9b0b435cf26e..bcb714031d69 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2352,6 +2352,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
>   
>   static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>   {
> +	int ret = 0;

Why is the initialization needed? You can as well declare the variable 
only inside the if below.

>   	int need_close = 0;
>   	int need_restart = 0;
>   
> @@ -2419,10 +2420,13 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>   	 * FIXME: We need to separate activation/deactivation from adding
>   	 * and removing from the mux array
>   	 */
> -	if (need_restart)
> -		gsm_activate_mux(gsm);
> -	if (gsm->initiator && need_close)
> -		gsm_dlci_begin_open(gsm->dlci[0]);
> +	if (gsm->dead) {
> +		ret = gsm_activate_mux(gsm);
> +		if (ret)
> +			return ret;
> +		if (gsm->initiator)
> +			gsm_dlci_begin_open(gsm->dlci[0]);
> +	}
>   	return 0;
>   }
>   


-- 
js
suse labs

[PATCH 3/3] tty: n_gsm: fix invalid gsmtty_write_room() result

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

gsmtty_write() does not prevent the user to use the full fifo size of 4096
bytes as allocated in gsm_dlci_alloc(). However, gsmtty_write_room() tries
to limit the return value by 'TX_SIZE' and returns a negative value if the
fifo has more than 'TX_SIZE' bytes stored. This is obviously wrong as
'TX_SIZE' is defined as 512.
Define 'TX_SIZE' to the fifo size and use it accordingly for allocation to
keep the current behavior. Return the correct remaining size of the fifo in
gsmtty_write_room() via kfifo_avail().

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index bcb714031d69..fd8b86dde525 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -137,6 +137,7 @@ struct gsm_dlci {
 	int retries;
 	/* Uplink tty if active */
 	struct tty_port port;	/* The tty bound to this DLCI if there is one */
+#define TX_SIZE		4096    /* Must be power of 2. */
 	struct kfifo fifo;	/* Queue fifo for the DLCI */
 	int adaption;		/* Adaption layer in use */
 	int prev_adaption;
@@ -1731,7 +1732,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 		return NULL;
 	spin_lock_init(&dlci->lock);
 	mutex_init(&dlci->mutex);
-	if (kfifo_alloc(&dlci->fifo, 4096, GFP_KERNEL) < 0) {
+	if (kfifo_alloc(&dlci->fifo, TX_SIZE, GFP_KERNEL) < 0) {
 		kfree(dlci);
 		return NULL;
 	}
@@ -2976,8 +2977,6 @@ static struct tty_ldisc_ops tty_ldisc_packet = {
  *	Virtual tty side
  */
 
-#define TX_SIZE		512
-
 /**
  *	gsm_modem_upd_via_data	-	send modem bits via convergence layer
  *	@dlci: channel
@@ -3217,7 +3216,7 @@ static unsigned int gsmtty_write_room(struct tty_struct *tty)
 	struct gsm_dlci *dlci = tty->driver_data;
 	if (dlci->state == DLCI_CLOSED)
 		return 0;
-	return TX_SIZE - kfifo_len(&dlci->fifo);
+	return kfifo_avail(&dlci->fifo);
 }
 
 static unsigned int gsmtty_chars_in_buffer(struct tty_struct *tty)
-- 
2.34.1

Re: [PATCH 3/3] tty: n_gsm: fix invalid gsmtty_write_room() result

[Jiri Slaby]

On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> gsmtty_write() does not prevent the user to use the full fifo size of 4096
> bytes as allocated in gsm_dlci_alloc(). However, gsmtty_write_room() tries
> to limit the return value by 'TX_SIZE' and returns a negative value if the
> fifo has more than 'TX_SIZE' bytes stored. This is obviously wrong as
> 'TX_SIZE' is defined as 512.
> Define 'TX_SIZE' to the fifo size and use it accordingly for allocation to
> keep the current behavior. Return the correct remaining size of the fifo in
> gsmtty_write_room() via kfifo_avail().

Right.

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>   drivers/tty/n_gsm.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index bcb714031d69..fd8b86dde525 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -137,6 +137,7 @@ struct gsm_dlci {
>   	int retries;
>   	/* Uplink tty if active */
>   	struct tty_port port;	/* The tty bound to this DLCI if there is one */
> +#define TX_SIZE		4096    /* Must be power of 2. */

Only that I'd not put the macro definition here. But outside the structure.

>   	struct kfifo fifo;	/* Queue fifo for the DLCI */
>   	int adaption;		/* Adaption layer in use */
>   	int prev_adaption;
> @@ -1731,7 +1732,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>   		return NULL;
>   	spin_lock_init(&dlci->lock);
>   	mutex_init(&dlci->mutex);
> -	if (kfifo_alloc(&dlci->fifo, 4096, GFP_KERNEL) < 0) {
> +	if (kfifo_alloc(&dlci->fifo, TX_SIZE, GFP_KERNEL) < 0) {
>   		kfree(dlci);
>   		return NULL;
>   	}
> @@ -2976,8 +2977,6 @@ static struct tty_ldisc_ops tty_ldisc_packet = {
>    *	Virtual tty side
>    */
>   
> -#define TX_SIZE		512
> -
>   /**
>    *	gsm_modem_upd_via_data	-	send modem bits via convergence layer
>    *	@dlci: channel
> @@ -3217,7 +3216,7 @@ static unsigned int gsmtty_write_room(struct tty_struct *tty)
>   	struct gsm_dlci *dlci = tty->driver_data;
>   	if (dlci->state == DLCI_CLOSED)
>   		return 0;
> -	return TX_SIZE - kfifo_len(&dlci->fifo);
> +	return kfifo_avail(&dlci->fifo);
>   }
>   
>   static unsigned int gsmtty_chars_in_buffer(struct tty_struct *tty)


-- 
js
suse labs

Re: [PATCH 1/3] tty: n_gsm: fix buffer over-read in gsm_dlci_data()

[Jiri Slaby]

On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> 'len' is decreased after each octet that has its EA bit set to 0, which
> means that the value is encoded with additional octets. However, the final
> octet does not decreases 'len' which results in 'len' being one byte too
> long. A buffer over-read may occur in tty_insert_flip_string() as it tries
> to read one byte more than the passed content size of 'data'.
> Decrease 'len' also for the final octet which has the EA bit set to 1 to
> write the correct number of bytes from the internal receive buffer to the
> virtual tty.
> 
> Fixes: 2e124b4a390c ("TTY: switch tty_flip_buffer_push")

That commit barely introduced the problem.

> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>   drivers/tty/n_gsm.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index a38b922bcbc1..9b0b435cf26e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1658,6 +1658,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
>   			if (len == 0)
>   				return;
>   		}
> +		len--;
>   		slen++;
>   		tty = tty_port_tty_get(port);
>   		if (tty) {


-- 
js
suse labs

RE: [PATCH 3/3] tty: n_gsm: fix invalid gsmtty_write_room() result

[Starke, Daniel]

> > +#define TX_SIZE		4096    /* Must be power of 2. */
> 
> Only that I'd not put the macro definition here. But outside the structure.
> 
> >   	struct kfifo fifo;	/* Queue fifo for the DLCI */

I have placed it at the field which it affects the same way as the original
author placed TX_THRESH_HI and TX_THRESH_LO at tx_list within struct gsm_mux.
I can resubmit this patch, but it was already included in the tty-linux
branch. Please let me know your opinion on this.

Best regards,
Daniel Starke