Re: [PATCH v42 18/98] hw/sd/sdcard: Introduce sd_cmd_to_sendingdata and sd_generic_read_byte

[Cédric Le Goater]

On 7/1/24 6:40 PM, Philippe Mathieu-Daudé wrote:
> On 28/6/24 09:44, Cédric Le Goater wrote:
> > On 6/28/24 9:00 AM, Philippe Mathieu-Daudé wrote:
> > > All commands switching from TRANSFER state to (sending)DATA
> > > do the same: send stream of data on the DAT lines. Instead
> > > of duplicating the same code many times, introduce 2 helpers:
> > > - sd_cmd_to_sendingdata() on the I/O line setup the data to
> > >    be transferred,
> > > - sd_generic_read_byte() on the DAT lines to fetch the data.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/sd/sd.c | 39 +++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 39 insertions(+)
> > >
> > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > > index d85b2906f4..1a8d06804d 100644
> > > --- a/hw/sd/sd.c
> > > +++ b/hw/sd/sd.c
> > > @@ -142,8 +142,10 @@ struct SDState {
> > >        */
> > >       bool expecting_acmd;
> > >       uint32_t blk_written;
> > > +
> > >       uint64_t data_start;
> > >       uint32_t data_offset;
> > > +    size_t data_size;
> > >       uint8_t data[512];
> > >       uint8_t vendor_data[512];
> > > @@ -1083,6 +1085,29 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, 
> > > SDRequest req)
> > >       return sd_illegal;
> > >   }
> > > +/* Configure fields for following sd_generic_read_byte() calls */
> > > +__attribute__((unused))
> > > +static sd_rsp_type_t sd_cmd_to_sendingdata(SDState *sd, SDRequest req,
> > > +                                           uint64_t start,
> > > +                                           const void *data, size_t size)
> > > +{
> > > +    if (sd->state != sd_transfer_state) {
> > > +        sd_invalid_state_for_cmd(sd, req);
> > > +    }
> > > +
> > > +    sd->state = sd_sendingdata_state;
> > > +    sd->data_start = start;
> > > +    sd->data_offset = 0;
> > > +    if (data) {
> > > +        assert(size);
> >
> > Shouldn't we check for buffer overrun ? sizeof(sd->data)
>
> OK if I squash this?
>
> -- >8 --
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d292e0adb5..f2d069c2da 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1123,7 +1123,7 @@ static sd_rsp_type_t sd_cmd_to_sendingdata(SDState *sd, 
> SDRequest req,
>       sd->data_start = start;
>       sd->data_offset = 0;
>       if (data) {
> -        assert(size);
> +        assert(size > 0 && size <= sizeof(sd->data));
>           memcpy(sd->data, data, size);
>       }
> ---

sure.

Thanks,

C.