Re: [Qemu-devel] [PATCH] sd: limit sd_cmd_type array index 'req.cmd'

[Peter Maydell <peter.maydell@linaro.org>]

On 20 January 2016 at 13:14, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While processing standard SD commands, the 'req.cmd' value could
> lead to OOB read when used as an index into 'sd_cmd_type' array.
> Limit 'req.cmd' value to avoid such an access.
>
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/sd/sd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a9935c..b800ced 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -668,8 +668,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>      /* Not interpreting this as an app command */
>      sd->card_status &= ~APP_CMD;
>
> -    if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc)
> +    if (sd_cmd_type[req.cmd & 0x3F] == sd_ac
> +        || sd_cmd_type[req.cmd & 0x3F] == sd_adtc) {
>          rca = req.arg >> 16;
> +    }
>
>      DPRINTF("CMD%d 0x%08x state %d\n", req.cmd, req.arg, sd->state);
>      switch (req.cmd) {

This isn't the only bit of sd.c that assumes that the controller
model hasn't passed it an oversize command number --
sd_do_command() calls cmd_valid_while_locked() which uses req.cmd
as an index into sd_cmd_class[].

(I think it's right for sd.c to be defensive about this, since
it's one place to get right rather than having to have all
the controller models DTRT.)

thanks
-- PMM