From: Daniel Starke <[email protected]>
'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: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
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
From: Daniel Starke <[email protected]>
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: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
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
From: Daniel Starke <[email protected]>
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: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
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
On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> 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 <[email protected]>
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> 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
On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> 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
On 04. 05. 22, 10:17, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> '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: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> 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
> On 04. 05. 22, 10:17, D. Starke wrote:
> > From: Daniel Starke <[email protected]>
> >
> > '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.
You are right. It was introduced in
commit e1eaea46bb40 ("tty: n_gsm line discipline")
This patch was already included in the tty-linus branch. Shall I resubmit it nevertheless?
Best regards,
Daniel Starke
> > 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.
You are right, this was unneeded. But this patch was already included in
the tty-linus branch. Shall I resubmit it nevertheless?
Best regards,
Daniel Starke
> > +#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
On 09. 05. 22, 13:03, Starke, Daniel wrote:
>>> 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.
>
> You are right, this was unneeded. But this patch was already included in
> the tty-linus branch. Shall I resubmit it nevertheless?
In that case, you can only send a fixup patch...
thanks,
--
js
suse labs