| 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
| ||
|
Date: Fri, 21 Oct 2022 13:09:57 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "D. Starke" <daniel.starke@...mens.com>
cc: linux-serial <linux-serial@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] tty: n_gsm: add parameter negotiation support
On Fri, 21 Oct 2022, D. Starke wrote:
> From: Daniel Starke <daniel.starke@...mens.com>
>
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapter 5.1.8.1.1 describes the parameter negotiation
> messages and parameters. Chapter 5.4.1 states that the default parameters
> are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
> the encoding of the parameter negotiation message. The meaning of the
> parameters and allowed value ranges can be found in chapter 5.7.
>
> Add parameter negotiation support accordingly. DLCI specific parameter
> configuration by the user requires additional ioctls. This is subject to another
> patch.
>
> Signed-off-by: Daniel Starke <daniel.starke@...mens.com>
> ---
> drivers/tty/n_gsm.c | 308 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 300 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index b6813a134c18..c6fe00afe1b2 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -123,6 +123,7 @@ struct gsm_msg {
>
> enum gsm_dlci_state {
> DLCI_CLOSED,
> + DLCI_CONFIGURE, /* Sending PN (for adaption > 1) */
> DLCI_OPENING, /* Sending SABM not seen UA */
> DLCI_OPEN, /* SABM/UA complete */
> DLCI_CLOSING, /* Sending DISC not seen UA/DM */
> @@ -406,6 +407,7 @@ static const u8 gsm_fcs8[256] = {
> #define INIT_FCS 0xFF
> #define GOOD_FCS 0xCF
>
> +static void gsm_dlci_close(struct gsm_dlci *dlci);
> static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
> static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
> static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
> @@ -528,6 +530,55 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
> kfree(prefix);
> }
>
> +/**
> + * gsm_encode_params - encode DLCI parameters
> + * @dlci: DLCI to encode from
> + * @data: 8 byte buffer for encoded data
> + * @dlen: length of buffer
> + *
> + * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
> + * table 3.
> + */
> +static int gsm_encode_params(const struct gsm_dlci *dlci, u8 *data,
> + unsigned int dlen)
> +{
> + struct gsm_mux *gsm = dlci->gsm;
> +
> + if (dlen < 8)
> + return -EINVAL;
Should this be MIN_MTU?
> + data[0] = dlci->addr;
> + data[1] = 0x00; /* UIH, convergence layer type 1 */
> + data[2] = dlci->prio;
> + data[3] = gsm->t1;
> + data[4] = dlci->mtu & 0xFF;
> + data[5] = (dlci->mtu >> 8) & 0xFF;
> + data[6] = gsm->n2;
> + data[7] = dlci->k;
Magic offsets, shouldn't you define a struct and assign to the named
fields (and use the correct endian type + accessors for that two-byte
field).
> + if (dlci->ftype == UI) {
> + data[1] = 0x01; /* UI */
> + } else if (dlci->ftype != UIH) {
> + pr_err("%s: unsupported frame type %d\n", __func__,
> + dlci->ftype);
> + return -EINVAL;
> + }
> +
> + switch (dlci->adaption) {
> + case 1: /* Unstructured */
> + break;
> + case 2: /* Unstructured with modem bits. */
> + data[1] |= 0x10; /* convergence layer type 2 */
> + break;
> + default:
> + pr_err("%s: unsupported adaption %d\n", __func__,
> + dlci->adaption);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /**
> * gsm_register_devices - register all tty devices for a given mux index
> *
> @@ -1445,6 +1496,122 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
> dlci->modem_rx = mlines;
> }
>
> +/**
> + * gsm_process_negotiation - process received parameters
> + * @gsm: GSM channel
> + * @addr: DLCI address
> + * @cr: command/response
> + * @data: data following command
> + * @clen: length of data
> + *
> + * Used when the response for our parameter negotiation command was
> + * received.
> + */
> +static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
> + unsigned int cr, const u8 *data,
> + unsigned int clen)
> +{
> + struct gsm_dlci *dlci = gsm->dlci[addr];
> + unsigned int ftype, i, adaption, prio, n1, k;
> +
> + if (clen < 8)
> + return -EINVAL;
> +
> + i = data[1] & 0x0F;
> + adaption = ((data[1] >> 4) & 0x0F) + 1;
> + prio = data[2] & 0x3F;
> + /* t1 = data[3]; */
> + n1 = data[4] | (data[5] << 8);
> + /* n2 = data[6]; */
> + k = data[7] & 0x07;
Magic offsets.
Define these fields properly too with names and GENMASK. Use FIELD_GET()
to extract values.
> + if (n1 < MIN_UNIT_SIZE) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s N1 out of range in PN\n", __func__);
> + return -EINVAL;
> + }
> +
> + switch (i) {
> + case 0x00:
> + ftype = UIH;
> + break;
> + case 0x01:
> + ftype = UI;
> + break;
> + case 0x02: /* I frames are not supported */
> + if (debug & DBG_ERRORS)
> + pr_info("%s unsupported I frame request in PN\n",
> + __func__);
> + return -EINVAL;
> + default:
> + if (debug & DBG_ERRORS)
> + pr_info("%s i out of range in PN\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (!cr && gsm->initiator) {
> + if (adaption != dlci->adaption) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid adaption %d in PN\n",
> + __func__, adaption);
> + return -EINVAL;
> + }
> + if (prio != dlci->prio) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid priority %d in PN",
> + __func__, prio);
> + return -EINVAL;
> + }
> + if (n1 > gsm->mru || n1 > dlci->mtu) {
> + /* We requested a frame size but the other party wants
> + * to send larger frames. The standard allows only a
> + * smaller response value than requested (5.4.6.3.1).
> + */
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid N1 %d in PN\n", __func__,
> + n1);
> + return -EINVAL;
> + }
> + dlci->mtu = n1;
> + if (ftype != dlci->ftype) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid i %d in PN\n", __func__, i);
> + return -EINVAL;
> + }
> + if (ftype != UI && ftype != UIH && k > dlci->k) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid k %d in PN\n", __func__, k);
> + return -EINVAL;
> + }
> + dlci->k = k;
> + } else if (cr && !gsm->initiator) {
> + /* Only convergence layer type 1 and 2 are supported. */
> + if (adaption != 1 && adaption != 2) {
> + if (debug & DBG_ERRORS)
> + pr_info("%s invalid adaption %d in PN\n",
> + __func__, adaption);
> + return -EINVAL;
> + }
> + dlci->adaption = adaption;
> + if (n1 > gsm->mru) {
> + /* Propose a smaller value */
> + dlci->mtu = gsm->mru;
> + } else if (n1 > MAX_MTU) {
> + /* Propose a smaller value */
> + dlci->mtu = MAX_MTU;
> + } else {
> + dlci->mtu = n1;
> + }
> + dlci->prio = prio;
> + dlci->ftype = ftype;
> + dlci->k = k;
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /**
> * gsm_control_modem - modem status received
> * @gsm: GSM channel
> @@ -1498,6 +1665,62 @@ static void gsm_control_modem(struct gsm_mux *gsm, const u8 *data, int clen)
> gsm_control_reply(gsm, CMD_MSC, data, clen);
> }
>
> +/**
> + * gsm_control_negotiation - parameter negotiation received
> + * @gsm: GSM channel
> + * @cr: command/response flag
> + * @data: data following command
> + * @dlen: data length
> + *
> + * We have received a parameter negotiation message. This is used by
> + * the GSM mux protocol to configure protocol parameters for a new DLCI.
> + */
> +static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
> + const u8 *data, unsigned int dlen)
> +{
> + unsigned int addr;
> + u8 params[8];
> + struct gsm_dlci *dlci;
> +
> + if (dlen < 8)
> + return;
MIN_MTU?
> +
> + /* Invalid DLCI? */
> + addr = data[0] & 0x3F;
#define + FIELD_GET()
> + if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr])
> + return;
> + dlci = gsm->dlci[addr];
> +
> + /* Too late for parameter negotiation? */
> + if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN)
> + return;
> +
> + /* Process the received parameters */
> + if (gsm_process_negotiation(gsm, addr, cr, data, dlen) != 0) {
> + /* Negotiation failed. Close the link. */
> + if (debug & DBG_ERRORS)
> + pr_info("%s PN failed\n", __func__);
> + gsm_dlci_close(dlci);
> + return;
> + }
> +
> + if (cr) {
> + /* Reply command with accepted parameters. */
> + if (gsm_encode_params(dlci, params, sizeof(params)) == 0)
> + gsm_control_reply(gsm, CMD_PN, params, sizeof(params));
> + else if (debug & DBG_ERRORS)
> + pr_info("%s PN invalid\n", __func__);
> + } else if (dlci->state == DLCI_CONFIGURE) {
> + /* Proceed with link setup by sending SABM before UA */
> + dlci->state = DLCI_OPENING;
> + gsm_command(gsm, dlci->addr, SABM|PF);
> + mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> + } else {
> + if (debug & DBG_ERRORS)
> + pr_info("%s PN in invalid state\n", __func__);
> + }
> +}
> +
> /**
> * gsm_control_rls - remote line status
> * @gsm: GSM channel
> @@ -1607,8 +1830,12 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
> /* Modem wishes to enter power saving state */
> gsm_control_reply(gsm, CMD_PSC, NULL, 0);
> break;
> + /* Optional commands */
> + case CMD_PN:
> + /* Modem sends a parameter negotiation command */
> + gsm_control_negotiation(gsm, 1, data, clen);
> + break;
> /* Optional unsupported commands */
> - case CMD_PN: /* Parameter negotiation */
> case CMD_RPN: /* Remote port negotiation */
> case CMD_SNC: /* Service negotiation command */
> default:
> @@ -1641,8 +1868,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
> spin_lock_irqsave(&gsm->control_lock, flags);
>
> ctrl = gsm->pending_cmd;
> - /* Does the reply match our command */
> command |= 1;
> + /* Does the reply match our command */
> if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
> /* Our command was replied to, kill the retry timer */
> del_timer(&gsm->t2_timer);
> @@ -1652,6 +1879,9 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
> ctrl->error = -EOPNOTSUPP;
> ctrl->done = 1;
> wake_up(&gsm->event);
> + /* Or did we receive the PN response to our PN command */
> + } else if (command == CMD_PN) {
> + gsm_control_negotiation(gsm, 0, data, clen);
> }
> spin_unlock_irqrestore(&gsm->control_lock, flags);
> }
> @@ -1829,6 +2059,31 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
> wake_up(&dlci->gsm->event);
> }
>
> +/**
> + * gsm_dlci_negotiate - start parameter negotiation
> + * @dlci: DLCI to open
> + *
> + * Starts the parameter negotiation for the new DLCI. This needs to be done
> + * before the DLCI initialized the channel via SABM.
> + */
> +static int gsm_dlci_negotiate(struct gsm_dlci *dlci)
> +{
> + struct gsm_mux *gsm = dlci->gsm;
> + u8 params[8];
> + int ret;
> +
> + ret = gsm_encode_params(dlci, params, sizeof(params));
> + if (ret != 0)
> + return ret;
> +
> + /* We cannot asynchronous wait for the command response with
> + * gsm_command() and gsm_control_wait() at this point.
> + */
> + ret = gsm_control_command(gsm, CMD_PN, params, sizeof(params));
> +
> + return ret;
> +}
> +
> /**
> * gsm_dlci_t1 - T1 timer expiry
> * @t: timer contained in the DLCI that opened
> @@ -1850,6 +2105,14 @@ static void gsm_dlci_t1(struct timer_list *t)
> struct gsm_mux *gsm = dlci->gsm;
>
> switch (dlci->state) {
> + case DLCI_CONFIGURE:
> + if (dlci->retries && gsm_dlci_negotiate(dlci) == 0) {
> + dlci->retries--;
> + mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
I'd tend to think a helper for this wouldn't hurt. There are already a
few of them.
> + } else {
> + gsm_dlci_begin_close(dlci); /* prevent half open link */
> + }
> + break;
> case DLCI_OPENING:
> if (dlci->retries) {
> dlci->retries--;
> @@ -1888,17 +2151,46 @@ static void gsm_dlci_t1(struct timer_list *t)
> * to the modem which should then reply with a UA or ADM, at which point
> * we will move into open state. Opening is done asynchronously with retry
> * running off timers and the responses.
> + * Parameter negotiation is performed before SABM if required.
> */
>
> static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
> {
> - struct gsm_mux *gsm = dlci->gsm;
> - if (dlci->state == DLCI_OPEN || dlci->state == DLCI_OPENING)
> + struct gsm_mux *gsm = dlci ? dlci->gsm : NULL;
> + bool need_pn = false;
> +
> + if (!gsm)
> return;
> - dlci->retries = gsm->n2;
> - dlci->state = DLCI_OPENING;
> - gsm_command(dlci->gsm, dlci->addr, SABM|PF);
> - mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> +
> + if (dlci->addr != 0) {
> + if (gsm->adaption != 1 || gsm->adaption != dlci->adaption)
> + need_pn = true;
> + if (dlci->prio != ((((dlci->addr / 8) + 1) * 8) - 1))
roundup(addr, 8) - 1 again?
--
i.
> + need_pn = true;
> + if (gsm->ftype != dlci->ftype)
> + need_pn = true;
> + }
> +
> + switch (dlci->state) {
> + case DLCI_CLOSED:
> + case DLCI_CLOSING:
> + dlci->retries = gsm->n2;
> + if (!need_pn) {
> + dlci->state = DLCI_OPENING;
> + gsm_command(gsm, dlci->addr, SABM|PF);
> + } else {
> + /* Configure DLCI before setup */
> + dlci->state = DLCI_CONFIGURE;
> + if (gsm_dlci_negotiate(dlci) != 0) {
> + gsm_dlci_close(dlci);
> + return;
> + }
> + }
> + mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> + break;
> + default:
> + break;
> + }
> }
>
> /**
>
Powered by blists - more mailing lists