Possible Null dereference in code.
I'm looking at some Linux code and something seems strange to me.
In the code below, it is not clear to me what the else clause achieves and whether it is broken...
Code:
} else {
/* Nothing to do. Next chunk in the packet, please. */
ch = (struct sctp_chunkhdr *)chunk->chunk_end;
/* Force chunk->skb->data to chunk->chunk_end. */
skb_pull(chunk->skb, chunk->chunk_end - chunk->skb->data);
/* We are guaranteed to pull a SCTP header. */
}
Isn't this the code above trying to deference a null pointer in the code below
Code:
struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
{
struct sctp_chunk *chunk;
struct sctp_chunkhdr *ch = NULL;
/* The assumption is that we are safe to process the chunks
* at this time.
*/
chunk = queue->in_progress;
if (chunk) {
/* There is a packet that we have been working on.
* Any post processing work to do before we move on?
*/
if (chunk->singleton ||
chunk->end_of_packet ||
chunk->pdiscard) {
if (chunk->head_skb == chunk->skb) {
chunk->skb = skb_shinfo(chunk->skb)->frag_list;
goto new_skb;
}
if (chunk->skb->next) {
chunk->skb = chunk->skb->next;
goto new_skb;
}
if (chunk->head_skb)
chunk->skb = chunk->head_skb;
sctp_chunk_free(chunk);
chunk = queue->in_progress = NULL;
} else {
/* Nothing to do. Next chunk in the packet, please. */
ch = (struct sctp_chunkhdr *)chunk->chunk_end;
/* Force chunk->skb->data to chunk->chunk_end. */
skb_pull(chunk->skb, chunk->chunk_end - chunk->skb->data);
/* We are guaranteed to pull a SCTP header. */
}
}
/* Do we need to take the next packet out of the queue to process? */
if (!chunk) {
struct list_head *entry;
next_chunk:
/* Is the queue empty? */
entry = sctp_list_dequeue(&queue->in_chunk_list);
if (!entry)
return NULL;
chunk = list_entry(entry, struct sctp_chunk, list);
if (skb_is_gso(chunk->skb) && skb_is_gso_sctp(chunk->skb)) {
/* GSO-marked skbs but without frags, handle
* them normally
*/
if (skb_shinfo(chunk->skb)->frag_list)
chunk->head_skb = chunk->skb;
/* skbs with "cover letter" */
if (chunk->head_skb && chunk->skb->data_len == chunk->skb->len)
chunk->skb = skb_shinfo(chunk->skb)->frag_list;
if (WARN_ON(!chunk->skb)) {
__SCTP_INC_STATS(dev_net(chunk->skb->dev), SCTP_MIB_IN_PKT_DISCARDS);
sctp_chunk_free(chunk);
goto next_chunk;
}
}
if (chunk->asoc)
sock_rps_save_rxhash(chunk->asoc->base.sk, chunk->skb);
queue->in_progress = chunk;
new_skb:
/* This is the first chunk in the packet. */
ch = (struct sctp_chunkhdr *)chunk->skb->data;
chunk->singleton = 1;
chunk->data_accepted = 0;
chunk->pdiscard = 0;
chunk->auth = 0;
chunk->has_asconf = 0;
chunk->end_of_packet = 0;
chunk->length = 0;
if (chunk->head_skb) {
struct sctp_input_cb
*cb = SCTP_INPUT_CB(chunk->skb),
*head_cb = SCTP_INPUT_CB(chunk->head_skb);
cb->chunk = head_cb->chunk;
cb->af = head_cb->af;
}
}
chunk->chunk_hdr = ch;
chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));
skb_pull(chunk->skb, sizeof(*ch));
chunk->subh.v = NULL; /* Subheader is no longer valid. */
if (chunk->chunk_end + sizeof(*ch) <= skb_tail_pointer(chunk->skb)) {
/* This is not a singleton */
chunk->singleton = 0;
} else if (chunk->chunk_end > skb_tail_pointer(chunk->skb)) {
/* Discard inside state machine. */
chunk->pdiscard = 1;
chunk->chunk_end = skb_tail_pointer(chunk->skb);
} else {
/* We are at the end of the packet, so mark the chunk
* in case we need to send a SACK.
*/
chunk->end_of_packet = 1;
}
pr_debug("+++sctp_inq_pop+++ chunk:%p[%s], length:%d, skb->len:%d\n",
chunk, sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)),
ntohs(chunk->chunk_hdr->length), chunk->skb->len);
return chunk;
}
I am not sure, myself, because no bugs have been reported yet related to this but it is possible that this code path never gets taken.
Any input is very much appreciated.
Thanks in advance!
