osmo-pcu: Re: New Defects reported by Coverity Scan for Osmocom

[Neels Hofmeyr]

On Thu, Dec 22, 2016 at 10:15:36AM -0800, scan-admin at coverity.com wrote:

> *** CID 158969:  Null pointer dereferences  (FORWARD_NULL)
> /source-Osmocom/osmo-pcu/src/gprs_rlcmac_sched.cpp: 131 in sched_select_ctrl_msg(unsigned char, unsigned char, unsigned int, unsigned char, gprs_rlcmac_pdch *, gprs_rlcmac_tbf *, gprs_rlcmac_tbf *, gprs_rlcmac_ul_tbf *)()
> 125     	struct msgb *msg = NULL;
> 126     	struct gprs_rlcmac_tbf *tbf = NULL;
> 127     	struct gprs_rlcmac_tbf *next_list[3] = { ul_ass_tbf, dl_ass_tbf, ul_ack_tbf };
> 128     
> 129     	for (size_t i = 0; i < ARRAY_SIZE(next_list); ++i) {
> 130     		tbf = next_list[(pdch->next_ctrl_prio + i) % 3];
> >>>     CID 158969:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Comparing "tbf" to null implies that "tbf" might be null.
> 131     		if (!tbf)
> 132     			continue;
> 133     
> 134     		/*
> 135     		 * Assignments for the same direction have lower precedence,
> 136     		 * because they may kill the TBF when the CONTROL ACK is

Even though coverity triggered on the recent addition of
  tbf->ms()->update_dl_ctrl_msg();
in osmo-pcu da7250ad2c1cd5ddc7d3c6e10435a00b357ef8f7, this problem has existed
before.  This function is handling the tbf pointer improperly. Let's take a
look:


static struct msgb *sched_select_ctrl_msg(
		    uint8_t trx, uint8_t ts, uint32_t fn,
		    uint8_t block_nr, struct gprs_rlcmac_pdch *pdch,
		    struct gprs_rlcmac_tbf *ul_ass_tbf,
		    struct gprs_rlcmac_tbf *dl_ass_tbf,
		    struct gprs_rlcmac_ul_tbf *ul_ack_tbf)
{
	struct msgb *msg = NULL;
	struct gprs_rlcmac_tbf *tbf = NULL;

This NULL init would only make sense if ARRAY_SIZE(next_list) were zero.
Will not happen.

	struct gprs_rlcmac_tbf *next_list[3] = { ul_ass_tbf, dl_ass_tbf, ul_ack_tbf };

	for (size_t i = 0; i < ARRAY_SIZE(next_list); ++i) {
		tbf = next_list[(pdch->next_ctrl_prio + i) % 3];
		if (!tbf)
			continue;

		/*
		 * Assignments for the same direction have lower precedence,
		 * because they may kill the TBF when the CONTROL ACK is
		 * received, thus preventing the others from being processed.
		 */

		if (tbf == ul_ass_tbf && tbf->direction == GPRS_RLCMAC_DL_TBF)
			if (tbf->ul_ass_state ==
					GPRS_RLCMAC_UL_ASS_SEND_ASS_REJ)
				msg = ul_ass_tbf->create_packet_access_reject();
			else
				msg = ul_ass_tbf->create_ul_ass(fn, ts);
		else if (tbf == dl_ass_tbf && tbf->direction == GPRS_RLCMAC_UL_TBF)
			msg = dl_ass_tbf->create_dl_ass(fn, ts);
		else if (tbf == ul_ack_tbf)
			msg = ul_ack_tbf->create_ul_ack(fn, ts);

		if (!msg) {
			tbf = NULL;

If this is not the last for() iteration, setting tbf = NULL makes no sense, it
will be set to next_list[..] at the start of the next iteration.

If this *is* the last iteration, tbf = NULL makes no sense either, because msg
is NULL and tbf will be set to dl_ass_tbf or ul_ass_tbf in the if (!msg) {}
case below.

			continue;
		}

		pdch->next_ctrl_prio += 1;
		pdch->next_ctrl_prio %= 3;
		break;
	}

	if (!msg) {
		/*
		 * If one of these is left, the response (CONTROL ACK) from the
		 * MS will kill the current TBF, only one of them can be
		 * non-NULL
		 */
		if (dl_ass_tbf) {
			tbf = dl_ass_tbf;
			msg = dl_ass_tbf->create_dl_ass(fn, ts);
		} else if (ul_ass_tbf) {
			tbf = ul_ass_tbf;
			msg = ul_ass_tbf->create_ul_ass(fn, ts);
		}
	}

At this point, tbf may be NULL, either from 'if (!tbf) continue;' in the last
iteration, or because dl_ass_tbf or ul_ass_tbf were NULL. Below code assumes
that tbf is non-NULL and will segfault. Add 'if (!tbf) return NULL' here?

	/* any message */
	if (msg) {
		tbf->rotate_in_list();
		LOGP(DRLCMACSCHED, LOGL_DEBUG, "Scheduling control "
			"message at RTS for %s (TRX=%d, TS=%d)\n",
			tbf_name(tbf), trx, ts);
		/* Updates the dl ctrl msg counter for ms */
		tbf->ms()->update_dl_ctrl_msg();
		return msg;
	}
	/* schedule PACKET PAGING REQUEST */
	msg = pdch->packet_paging_request();
	if (msg) {
		LOGP(DRLCMACSCHED, LOGL_DEBUG, "Scheduling paging request "
			"message at RTS for (TRX=%d, TS=%d)\n", trx, ts);

		/* Updates the dl ctrl msg counter for ms */
		tbf->ms()->update_dl_ctrl_msg();
		return msg;
	}

	return NULL;
}


@osmo-pcu tbf folks, please fix this potential segmentation fault ASAP.
Thanks!

~N

-- 
- Neels Hofmeyr <nhofmeyr at sysmocom.de>          http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Harald Welte
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/osmocom-net-gprs/attachments/20161223/7e5ee9a5/attachment.bin>