| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFpK11BZDT_NmWyMe6Tvj8P3gJNBpM_hX5d52urG+sn6-g@mail.gmail.com>
Date: Tue, 15 Jul 2025 17:54:24 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Benoît Monin <benoit.monin@...tlin.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops
> > > +}
> > > +
> > > #endif
> > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> > > index 66283825513cb..848d8aa3ff2b5 100644
> > > --- a/drivers/mmc/core/mmc_ops.c
> > > +++ b/drivers/mmc/core/mmc_ops.c
> > > @@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
> > > return err;
> > > }
> > > EXPORT_SYMBOL_GPL(mmc_sanitize);
> > > +
> > > +/**
> > > + * mmc_read_blocks() - read data blocks from the mmc
> > > + * @card: mmc card to read from, can be NULL
> > > + * @host: mmc host doing the read
> > > + * @blksz: data block size
> > > + * @blocks: number of blocks to read
> > > + * @blk_addr: first block address
> > > + * @buf: output buffer
> > > + * @len: size of the buffer
> > > + *
> > > + * Read one or more blocks of data from the mmc. This is a low-level helper for
> > > + * tuning operation. If card is NULL, it is assumed that CMD23 can be used for
> > > + * multi-block read.
> > > + *
> > > + * Return: 0 in case of success, otherwise -EIO
> > > + */
> > > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > > + unsigned int blksz, unsigned int blocks,
> > > + unsigned int blk_addr, void *buf, unsigned int len)
> > > +{
> > > + struct mmc_request mrq = {};
> > > + struct mmc_command sbc = {};
> > > + struct mmc_command cmd = {};
> > > + struct mmc_command stop = {};
> > > + struct mmc_data data = {};
> > > + struct scatterlist sg;
> > > +
> > > + if (blocks > 1) {
> > > + if (mmc_host_can_cmd23(host) &&
> > > + (!card || mmc_card_can_cmd23(card))) {
> > > + mrq.sbc = &sbc;
> > > + sbc.opcode = MMC_SET_BLOCK_COUNT;
> > > + sbc.arg = blocks;
> > > + sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> > > + }
> > > + cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> > > + mrq.stop = &stop;
> > > + stop.opcode = MMC_STOP_TRANSMISSION;
> > > + stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> > > + } else {
> > > + cmd.opcode = MMC_READ_SINGLE_BLOCK;
> > > + }
> > > +
> > > + mrq.cmd = &cmd;
> > > + cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > > +
> > > + mrq.data = &data;
> > > + data.flags = MMC_DATA_READ;
> > > + data.blksz = blksz;
> > > + data.blocks = blocks;
> > > + data.blk_addr = blk_addr;
> > > + data.sg = &sg;
> > > + data.sg_len = 1;
> > > + if (card)
> > > + mmc_set_data_timeout(&data, card);
> > > + else
> > > + data.timeout_ns = 1000000000;
> > > +
> > > + sg_init_one(&sg, buf, len);
> > > +
> > > + mmc_wait_for_req(host, &mrq);
> > > +
> > > + if (sbc.error || cmd.error || data.error)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(mmc_read_blocks);
> > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > > index 68f09a955a902..72196817a6f0f 100644
> > > --- a/include/linux/mmc/host.h
> > > +++ b/include/linux/mmc/host.h
> > > @@ -743,5 +743,8 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
> > > int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> > > int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
> > > int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> > > +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> > > + unsigned int blksz, unsigned int blocks,
> > > + unsigned int blk_addr, void *buf, unsigned int len);
> >
> > I really think we must avoid exporting such a generic function. This
> > becomes visible outside the mmc subsystem and I am worried that it
> > will be abused.
> >
> > Can we perhaps make it harder to integrate with the tuning support on
> > the core, somehow? I haven't thought much about it, but maybe you can
> > propose something along those lines - otherwise I will try to think of
> > another way to do it.
> >
> I agree that the function might be too generic now. Here are some of
> the ideas I have to make less appealing for abuse:
>
> * Rename it to mention tuning (mmc_tuning_read?)
Yes, something like that or possibly "mmc_read_tuning".
> * Drop some parameters:
> * blk_addr: Reading from 0 should be all that is needed for tuning
> * other?
Yes, I like that. If we can make it useful only for the tuning case,
that would be great.
I think we should drop struct mmc_card* too. The ->execute_tuning()
host ops takes a struct mmc_host* and during initialization when the
callback is invoked from the core, host->card has not yet been set. In
other words, there are no "card" available for the host to use.
Moreover, do we really need to pass along the data that we have read?
Can't we just alloc the buffer, read the data and free the buffer. The
important part is to provide an error code to the caller, letting it
know whether we succeeded in reading the data or whether it failed,
right?
> * Move its declaration to a header private to drivers/mmc (where?)
Unfortunately not, we currently don't have any other suitable place,
but next to mmc_get_ext_csd().
>
> Let me know what you think.
>
Kind regards
Uffe
Powered by blists - more mailing lists