Hello,
+-- On Tue, 25 Oct 2016, Peter Maydell wrote --+
| > case 2: /* Packet Number Register */
| > - s->packet_num = value;
| > + s->packet_num = value & (NUM_PACKETS - 1);
|
| The data sheet says that there are 6 valid bits in the
| packet number register, not 3, which suggests masking to
| NUM_PACKETS-1 here isn't the right thing.
NUM_PACKETS macro is used at most places to limit access into 'data' array.
| Q: what about attempts to use packet numbers that have not
| been allocated by the 91c111's MMU ?
Added 'n & s->allocated' in patch v2.
| In any case, rather than doing this check on p I would
| suggest that we should do:
|
| p = s->ptr;
| if (s->ptr & 0x4000) {
| s->ptr = (s->ptr & 0xf800) | ((s->ptr + 1) & 0x7ff);
| } else {
| p += (offset & 3);
| }
| p &= 0x7ff;
|
| (ie move the mask operation down a bit), which will ensure p is
| always within bounds. Ditto in read code.
Done in patch v2.
| > + return 0x80;
|
| Why 0x80 ?
Changed it to 0.
| There's also an access to s->data[] in smc91c111_do_tx() which needs
| some kind of guard.
Rotuine 'smc91c111_queue_tx' which calls 'smc91c111_do_tx' has a guard in
place before calling _do_tx,
static void smc91c111_queue_tx(smc91c111_state *s, int packet)
{
if (s->tx_fifo_len == NUM_PACKETS)
return;
s->tx_fifo[s->tx_fifo_len++] = packet;
smc91c111_do_tx(s);
}
| smc91c111_release_packet() also assumes the packet number it
| is passed is sane.
It seems to have check in place for s->allocated which checks if given
packet number is allocated.
| Do we need to guard against bad packet numbers in incoming
| VMState data? The answer is no if we're using the "always
| check packet_num at point of use" approach, but yes if you're
| trying to ensure s->packet_num is always a valid value.
IMO second is better.
| Do we need to sanitize s->allocated in incoming vmstate data?
Not sure. It does not seem to be set by user.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
