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

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

[Starke, Daniel]

> >   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

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

[Jiri Slaby]

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

[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