diff mbox series

[v4,1/2] Add SAFE_MPROTECT() macro

Message ID 20240304152119.31688-2-andrea.cervesato@suse.de
State Superseded
Headers show
Series SysV IPC bug reproducer | expand

Commit Message

Andrea Cervesato March 4, 2024, 3:21 p.m. UTC 
From: Andrea Cervesato <andrea.cervesato@suse.com>

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
TDEBUG for mprotect

 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

Comments

Cyril Hrubis March 4, 2024, 3:54 p.m. UTC | #1 
Hi!
> +	int rval;
> +	char prot_buf[16];
> +
> +	switch (prot) {
> +	case PROT_NONE:
> +		snprintf(prot_buf, 16, "PROT_NONE");
> +		break;
> +	case PROT_WRITE:
> +		snprintf(prot_buf, 16, "PROT_WRITE");
> +		break;
> +	case PROT_READ:
> +		snprintf(prot_buf, 16, "PROT_READ");
> +		break;
> +	case PROT_EXEC:
> +		snprintf(prot_buf, 16, "PROT_EXEC");
> +		break;
> +	default:
> +		snprintf(prot_buf, 16, "UNKNOWN");
> +		break;
> +	}

This is ugly and does not work.

First of all we can just do:

	char *prot_name;


	switch (prot) {
	case PROT_NONE:
		prot_name = "PROT_NONE";
	break;
	...
	}

And secondly it does not work for common combinations, like (PROT_READ | PROT_WRITE).

So I guess that the easiest solution is to walk the bits and append the
string something as:

#define PROT_READ_NAME "PROT_READ | "
#define PROT_WRITE_NAME "PROT_WRITE | "
#define PROT_EXEC_NAME "PROT_EXEC | "

static const char *prot_to_str(int prot, char *buf, size_t buf_len)
{
	char *orig_buf = buf;

	if (buf_len < sizeof(PROT_READ_NAME) + sizeof(PROT_WRITE_NAME) + sizeof(PROT_EXEC_NAME))
		return "BUFFER TOO SMALL!!!";

	if (prot == 0)
		return "PROT_NONE";

	*buf = 0;

	if (prot & PROT_READ) {
		strcpy(buf, PROT_READ_NAME);
		buf += sizeof(PROT_READ_NAME)-1;
	}

	if (prot & PROT_WRITE) {
		strcpy(buf, PROT_WRITE_NAME);
		buf += sizeof(PROT_WRITE_NAME)-1;
	}

	if (prot & PROT_EXEC) {
		strcpy(buf, PROT_EXEC_NAME);
		buf += sizeof(PROT_EXEC_NAME)-1;
	}

	if (orig_buf != buf)
		buf[-3] = 0;

	return buf;
}

Also I would put the code that translates the prot into string into a
separate function because that code should be used in the safe_mmap() as
well.

> +	tst_res_(file, lineno, TDEBUG,
> +		"mprotect(%p, %d, %s)", addr, len, prot_buf);

Can we print hexadecimal value of the prot here as well?

		"mprotect(%p, %d, %s(%x))", addr, len, prot_to_str(prot, buf, sizeof(buf)), prot);

> +	rval = mprotect(addr, len, prot);
> +
> +	if (rval == -1) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"mprotect() failed");
> +	} else if (rval) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"Invalid mprotect() return value %d", rval);
> +	}
> +
> +	return rval;
> +}
> +
>  int safe_mincore(const char *file, const int lineno, void *start,
>  	size_t length, unsigned char *vec)
>  {
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
Petr Vorel March 4, 2024, 4:35 p.m. UTC | #2 
Hi,

> Hi!
> > +	int rval;
> > +	char prot_buf[16];
> > +
> > +	switch (prot) {
> > +	case PROT_NONE:
> > +		snprintf(prot_buf, 16, "PROT_NONE");
> > +		break;
> > +	case PROT_WRITE:
> > +		snprintf(prot_buf, 16, "PROT_WRITE");
> > +		break;
> > +	case PROT_READ:
> > +		snprintf(prot_buf, 16, "PROT_READ");
> > +		break;
> > +	case PROT_EXEC:
> > +		snprintf(prot_buf, 16, "PROT_EXEC");
> > +		break;
> > +	default:
> > +		snprintf(prot_buf, 16, "UNKNOWN");
> > +		break;
> > +	}

> This is ugly and does not work.

> First of all we can just do:

> 	char *prot_name;


> 	switch (prot) {
> 	case PROT_NONE:
> 		prot_name = "PROT_NONE";
> 	break;
> 	...
> 	}

> And secondly it does not work for common combinations, like (PROT_READ | PROT_WRITE).

> So I guess that the easiest solution is to walk the bits and append the
> string something as:

> #define PROT_READ_NAME "PROT_READ | "
> #define PROT_WRITE_NAME "PROT_WRITE | "
> #define PROT_EXEC_NAME "PROT_EXEC | "

nit: maybe using stringification?

#define FLAG(f) f, #f " | "

FLAG(PROT_READ)

> static const char *prot_to_str(int prot, char *buf, size_t buf_len)
> {
> 	char *orig_buf = buf;

> 	if (buf_len < sizeof(PROT_READ_NAME) + sizeof(PROT_WRITE_NAME) + sizeof(PROT_EXEC_NAME))
> 		return "BUFFER TOO SMALL!!!";

> 	if (prot == 0)
> 		return "PROT_NONE";

> 	*buf = 0;

> 	if (prot & PROT_READ) {
> 		strcpy(buf, PROT_READ_NAME);
> 		buf += sizeof(PROT_READ_NAME)-1;
> 	}

> 	if (prot & PROT_WRITE) {
> 		strcpy(buf, PROT_WRITE_NAME);
> 		buf += sizeof(PROT_WRITE_NAME)-1;
> 	}

> 	if (prot & PROT_EXEC) {
> 		strcpy(buf, PROT_EXEC_NAME);
> 		buf += sizeof(PROT_EXEC_NAME)-1;
> 	}

> 	if (orig_buf != buf)
> 		buf[-3] = 0;

> 	return buf;
> }

> Also I would put the code that translates the prot into string into a
> separate function because that code should be used in the safe_mmap() as
> well.

+1

> > +	tst_res_(file, lineno, TDEBUG,
> > +		"mprotect(%p, %d, %s)", addr, len, prot_buf);

> Can we print hexadecimal value of the prot here as well?

+1

> 		"mprotect(%p, %d, %s(%x))", addr, len, prot_to_str(prot, buf, sizeof(buf)), prot);

Kind regards,
Petr
diff mbox series

Patch

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index f2ce8919b..bd401f094 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -625,6 +625,11 @@  int safe_munlock(const char *file, const int lineno, const char *addr,
 	size_t len);
 #define SAFE_MUNLOCK(addr, len) safe_munlock(__FILE__, __LINE__, (addr), (len))
 
+int safe_mprotect(const char *file, const int lineno, const char *addr,
+	size_t len, int prot);
+#define SAFE_MPROTECT(addr, len, prot) \
+	safe_mprotect(__FILE__, __LINE__, (addr), (len), (prot))
+
 int safe_mincore(const char *file, const int lineno, void *start,
 	size_t length, unsigned char *vec);
 #define SAFE_MINCORE(start, length, vec) \
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 951e1b064..07d15c9a7 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -1317,6 +1317,46 @@  int safe_munlock(const char *file, const int lineno, const void *addr,
 	return rval;
 }
 
+int safe_mprotect(const char *file, const int lineno, void *addr,
+	size_t len, int prot)
+{
+	int rval;
+	char prot_buf[16];
+
+	switch (prot) {
+	case PROT_NONE:
+		snprintf(prot_buf, 16, "PROT_NONE");
+		break;
+	case PROT_WRITE:
+		snprintf(prot_buf, 16, "PROT_WRITE");
+		break;
+	case PROT_READ:
+		snprintf(prot_buf, 16, "PROT_READ");
+		break;
+	case PROT_EXEC:
+		snprintf(prot_buf, 16, "PROT_EXEC");
+		break;
+	default:
+		snprintf(prot_buf, 16, "UNKNOWN");
+		break;
+	}
+
+	tst_res_(file, lineno, TDEBUG,
+		"mprotect(%p, %d, %s)", addr, len, prot_buf);
+
+	rval = mprotect(addr, len, prot);
+
+	if (rval == -1) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"mprotect() failed");
+	} else if (rval) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"Invalid mprotect() return value %d", rval);
+	}
+
+	return rval;
+}
+
 int safe_mincore(const char *file, const int lineno, void *start,
 	size_t length, unsigned char *vec)
 {