RE: [PATCH] aacraid: prevent out-of-bounds access due to changing fip header sizes

[David Carroll <david.carroll@xxxxxxxxxxxxx>]

[snip]
> > >                 goto cleanup;
> > >         }
> > >
> > > +       if (size != le16_to_cpu(kfib->header.Size)
> > > +                       + sizeof(struct aac_fibhdr)) {
> > > +               retval = -EINVAL;
> > > +               goto cleanup;
> > > +       }
> > > +
> > >         if (kfib->header.Command == cpu_to_le16(TakeABreakPt)) {
> > >                 aac_adapter_interrupt(dev);
> > >                 /*
> > > --
> > > 1.8.5.6
> > >
> >
> > NAK, size is the MAX((header.size+hdr), sender_size). I will send a patch
> tomorrow which will insure that neither of those values is larger than size on the
> second fetch.
> >
> > Thanks, -Dave
> 
> 
> OK, not sure if I understood you correctly, did you mean something like that:
> 
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index b381b37..a671b54 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -118,6 +118,10 @@ static int ioctl_send_fib(struct aac_dev * dev, void
> __user *arg)
>                 goto cleanup;
>         }
> 
> +       size = max((le16_to_cpu(kfib->header.Size)
> +                      + sizeof(struct aac_fibhdr)),
> +                     le16_to_cpu(kfib->header.SenderSize));
> +
>         if (kfib->header.Command == cpu_to_le16(TakeABreakPt)) {
>                 aac_adapter_interrupt(dev);
>                 /*
> @@ -127,7 +131,7 @@ static int ioctl_send_fib(struct aac_dev * dev, void
> __user *arg)
>                 kfib->header.XferState = 0;
>         } else {
>                 retval = aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> -                               le16_to_cpu(kfib->header.Size) , FsaNormal,
> +                               size , FsaNormal,
>                                 1, 1, NULL, NULL);
>                 if (retval) {
>                         goto cleanup;

Hi Johannes,

The max that I was referring to was earlier in the function;

	 */
	size = le16_to_cpu(kfib->header.Size) + sizeof(struct aac_fibhdr);
	if (size < le16_to_cpu(kfib->header.SenderSize))
		size = le16_to_cpu(kfib->header.SenderSize);

Usually, SenderSize is the amount of space available for return data, while Size if the command size. 
Your original patch would have caused an error if the application was expecting any return data, larger than
The space of the command. For example, a container command is about 50 bytes, while arcconf would set
SenderSize to 512-sizeof(struct aac_fibhdr). I think you will get a better idea with the patch I'll send today.

Thanks, -Dave
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html